홍길동 / onos

Go to a project
Toggle navigation Toggle navigation pinning
Sho SHIMIZU
Committed by Gerrit Code Review
1e0a34ce 1 parent ba2d3b78

Fix bug that true is always returned even in failure cases 

Additionally, remove catch block handling TransactionException, which
doesn't have to be caught in user side any more.

Change-Id: I359b50dbe0e1074a2bc4c8850459cb4463669aa8
Inline Side-by-side
Showing 2 changed files with 64 additions and 101 deletions
...@@ -29,7 +29,6 @@ import org.onosproject.store.service.ConsistentMap; ...@@ -29,7 +29,6 @@ import org.onosproject.store.service.ConsistentMap;
29 import org.onosproject.store.service.Serializer; 29 import org.onosproject.store.service.Serializer;
30 import org.onosproject.store.service.StorageService; 30 import org.onosproject.store.service.StorageService;
31 import org.onosproject.store.service.TransactionContext; 31 import org.onosproject.store.service.TransactionContext;
32 -import org.onosproject.store.service.TransactionException;
33 import org.onosproject.store.service.TransactionalMap; 32 import org.onosproject.store.service.TransactionalMap;
34 import org.onosproject.store.service.Versioned; 33 import org.onosproject.store.service.Versioned;
35 import org.slf4j.Logger; 34 import org.slf4j.Logger;
...@@ -100,29 +99,24 @@ public class ConsistentResourceStore implements ResourceStore { ...@@ -100,29 +99,24 @@ public class ConsistentResourceStore implements ResourceStore {
100 TransactionContext tx = service.transactionContextBuilder().build(); 99 TransactionContext tx = service.transactionContextBuilder().build();
101 tx.begin(); 100 tx.begin();
102 101
103 - try { 102 + TransactionalMap<ResourcePath, List<ResourcePath>> childTxMap =
104 - TransactionalMap<ResourcePath, List<ResourcePath>> childTxMap = 103 + tx.getTransactionalMap(CHILD_MAP, SERIALIZER);
105 - tx.getTransactionalMap(CHILD_MAP, SERIALIZER);
106 104
107 - Map<ResourcePath, List<ResourcePath>> resourceMap = resources.stream() 105 + Map<ResourcePath, List<ResourcePath>> resourceMap = resources.stream()
108 - .filter(x -> x.parent().isPresent()) 106 + .filter(x -> x.parent().isPresent())
109 - .collect(Collectors.groupingBy(x -> x.parent().get())); 107 + .collect(Collectors.groupingBy(x -> x.parent().get()));
110 108
111 - for (Map.Entry<ResourcePath, List<ResourcePath>> entry: resourceMap.entrySet()) { 109 + for (Map.Entry<ResourcePath, List<ResourcePath>> entry: resourceMap.entrySet()) {
112 - if (!isRegistered(childTxMap, entry.getKey())) { 110 + if (!isRegistered(childTxMap, entry.getKey())) {
113 - return abortTransaction(tx); 111 + return abortTransaction(tx);
114 - }
115 -
116 - if (!appendValues(childTxMap, entry.getKey(), entry.getValue())) {
117 - return abortTransaction(tx);
118 - }
119 } 112 }
120 113
121 - return commitTransaction(tx); 114 + if (!appendValues(childTxMap, entry.getKey(), entry.getValue())) {
122 - } catch (TransactionException e) { 115 + return abortTransaction(tx);
123 - log.error("Exception thrown, abort the transaction", e); 116 + }
124 - return abortTransaction(tx);
125 } 117 }
118 +
119 + return tx.commit();
126 } 120 }
127 121
128 @Override 122 @Override
...@@ -132,33 +126,28 @@ public class ConsistentResourceStore implements ResourceStore { ...@@ -132,33 +126,28 @@ public class ConsistentResourceStore implements ResourceStore {
132 TransactionContext tx = service.transactionContextBuilder().build(); 126 TransactionContext tx = service.transactionContextBuilder().build();
133 tx.begin(); 127 tx.begin();
134 128
135 - try { 129 + TransactionalMap<ResourcePath, List<ResourcePath>> childTxMap =
136 - TransactionalMap<ResourcePath, List<ResourcePath>> childTxMap = 130 + tx.getTransactionalMap(CHILD_MAP, SERIALIZER);
137 - tx.getTransactionalMap(CHILD_MAP, SERIALIZER); 131 + TransactionalMap<ResourcePath, ResourceConsumer> consumerTxMap =
138 - TransactionalMap<ResourcePath, ResourceConsumer> consumerTxMap = 132 + tx.getTransactionalMap(CONSUMER_MAP, SERIALIZER);
139 - tx.getTransactionalMap(CONSUMER_MAP, SERIALIZER); 133 +
140 - 134 + Map<ResourcePath, List<ResourcePath>> resourceMap = resources.stream()
141 - Map<ResourcePath, List<ResourcePath>> resourceMap = resources.stream() 135 + .filter(x -> x.parent().isPresent())
142 - .filter(x -> x.parent().isPresent()) 136 + .collect(Collectors.groupingBy(x -> x.parent().get()));
143 - .collect(Collectors.groupingBy(x -> x.parent().get())); 137 +
144 - 138 + // even if one of the resources is allocated to a consumer,
145 - // even if one of the resources is allocated to a consumer, 139 + // all unregistrations are regarded as failure
146 - // all unregistrations are regarded as failure 140 + for (Map.Entry<ResourcePath, List<ResourcePath>> entry: resourceMap.entrySet()) {
147 - for (Map.Entry<ResourcePath, List<ResourcePath>> entry: resourceMap.entrySet()) { 141 + if (entry.getValue().stream().anyMatch(x -> consumerTxMap.get(x) != null)) {
148 - if (entry.getValue().stream().anyMatch(x -> consumerTxMap.get(x) != null)) { 142 + return abortTransaction(tx);
149 - return abortTransaction(tx);
150 - }
151 -
152 - if (!removeValues(childTxMap, entry.getKey(), entry.getValue())) {
153 - return abortTransaction(tx);
154 - }
155 } 143 }
156 144
157 - return commitTransaction(tx); 145 + if (!removeValues(childTxMap, entry.getKey(), entry.getValue())) {
158 - } catch (TransactionException e) { 146 + return abortTransaction(tx);
159 - log.error("Exception thrown, abort the transaction", e); 147 + }
160 - return abortTransaction(tx);
161 } 148 }
149 +
150 + return tx.commit();
162 } 151 }
163 152
164 @Override 153 @Override
...@@ -169,28 +158,23 @@ public class ConsistentResourceStore implements ResourceStore { ...@@ -169,28 +158,23 @@ public class ConsistentResourceStore implements ResourceStore {
169 TransactionContext tx = service.transactionContextBuilder().build(); 158 TransactionContext tx = service.transactionContextBuilder().build();
170 tx.begin(); 159 tx.begin();
171 160
172 - try { 161 + TransactionalMap<ResourcePath, List<ResourcePath>> childTxMap =
173 - TransactionalMap<ResourcePath, List<ResourcePath>> childTxMap = 162 + tx.getTransactionalMap(CHILD_MAP, SERIALIZER);
174 - tx.getTransactionalMap(CHILD_MAP, SERIALIZER); 163 + TransactionalMap<ResourcePath, ResourceConsumer> consumerTxMap =
175 - TransactionalMap<ResourcePath, ResourceConsumer> consumerTxMap = 164 + tx.getTransactionalMap(CONSUMER_MAP, SERIALIZER);
176 - tx.getTransactionalMap(CONSUMER_MAP, SERIALIZER); 165 +
177 - 166 + for (ResourcePath resource: resources) {
178 - for (ResourcePath resource: resources) { 167 + if (!isRegistered(childTxMap, resource)) {
179 - if (!isRegistered(childTxMap, resource)) { 168 + return abortTransaction(tx);
180 - return abortTransaction(tx);
181 - }
182 -
183 - ResourceConsumer oldValue = consumerTxMap.put(resource, consumer);
184 - if (oldValue != null) {
185 - return abortTransaction(tx);
186 - }
187 } 169 }
188 170
189 - return commitTransaction(tx); 171 + ResourceConsumer oldValue = consumerTxMap.put(resource, consumer);
190 - } catch (TransactionException e) { 172 + if (oldValue != null) {
191 - log.error("Exception thrown, abort the transaction", e); 173 + return abortTransaction(tx);
192 - return abortTransaction(tx); 174 + }
193 } 175 }
176 +
177 + return tx.commit();
194 } 178 }
195 179
196 @Override 180 @Override
...@@ -202,28 +186,23 @@ public class ConsistentResourceStore implements ResourceStore { ...@@ -202,28 +186,23 @@ public class ConsistentResourceStore implements ResourceStore {
202 TransactionContext tx = service.transactionContextBuilder().build(); 186 TransactionContext tx = service.transactionContextBuilder().build();
203 tx.begin(); 187 tx.begin();
204 188
205 - try { 189 + TransactionalMap<ResourcePath, ResourceConsumer> consumerTxMap =
206 - TransactionalMap<ResourcePath, ResourceConsumer> consumerTxMap = 190 + tx.getTransactionalMap(CONSUMER_MAP, SERIALIZER);
207 - tx.getTransactionalMap(CONSUMER_MAP, SERIALIZER); 191 + Iterator<ResourcePath> resourceIte = resources.iterator();
208 - Iterator<ResourcePath> resourceIte = resources.iterator(); 192 + Iterator<ResourceConsumer> consumerIte = consumers.iterator();
209 - Iterator<ResourceConsumer> consumerIte = consumers.iterator(); 193 +
210 - 194 + while (resourceIte.hasNext() && consumerIte.hasNext()) {
211 - while (resourceIte.hasNext() && consumerIte.hasNext()) { 195 + ResourcePath resource = resourceIte.next();
212 - ResourcePath resource = resourceIte.next(); 196 + ResourceConsumer consumer = consumerIte.next();
213 - ResourceConsumer consumer = consumerIte.next();
214 -
215 - // if this single release fails (because the resource is allocated to another consumer,
216 - // the whole release fails
217 - if (!consumerTxMap.remove(resource, consumer)) {
218 - return abortTransaction(tx);
219 - }
220 - }
221 197
222 - return commitTransaction(tx); 198 + // if this single release fails (because the resource is allocated to another consumer,
223 - } catch (TransactionException e) { 199 + // the whole release fails
224 - log.error("Exception thrown, abort the transaction", e); 200 + if (!consumerTxMap.remove(resource, consumer)) {
225 - return abortTransaction(tx); 201 + return abortTransaction(tx);
202 + }
226 } 203 }
204 +
205 + return tx.commit();
227 } 206 }
228 207
229 @Override 208 @Override
...@@ -278,17 +257,6 @@ public class ConsistentResourceStore implements ResourceStore { ...@@ -278,17 +257,6 @@ public class ConsistentResourceStore implements ResourceStore {
278 } 257 }
279 258
280 /** 259 /**
281 - * Commit the transaction.
282 - *
283 - * @param tx transaction context
284 - * @return always true
285 - */
286 - private boolean commitTransaction(TransactionContext tx) {
287 - tx.commit();
288 - return true;
289 - }
290 -
291 - /**
292 * Appends the values to the existing values associated with the specified key. 260 * Appends the values to the existing values associated with the specified key.
293 * If the map already has all the given values, appending will not happen. 261 * If the map already has all the given values, appending will not happen.
294 * 262 *
......
...@@ -59,7 +59,6 @@ import org.onosproject.store.service.ConsistentMap; ...@@ -59,7 +59,6 @@ import org.onosproject.store.service.ConsistentMap;
59 import org.onosproject.store.service.Serializer; 59 import org.onosproject.store.service.Serializer;
60 import org.onosproject.store.service.StorageService; 60 import org.onosproject.store.service.StorageService;
61 import org.onosproject.store.service.TransactionContext; 61 import org.onosproject.store.service.TransactionContext;
62 -import org.onosproject.store.service.TransactionException;
63 import org.onosproject.store.service.TransactionalMap; 62 import org.onosproject.store.service.TransactionalMap;
64 import org.onosproject.store.service.Versioned; 63 import org.onosproject.store.service.Versioned;
65 64
...@@ -294,7 +293,7 @@ public class ConsistentLinkResourceStore extends ...@@ -294,7 +293,7 @@ public class ConsistentLinkResourceStore extends
294 intentAllocs.put(allocations.intentId(), allocations); 293 intentAllocs.put(allocations.intentId(), allocations);
295 allocations.links().forEach(link -> allocateLinkResource(tx, link, allocations)); 294 allocations.links().forEach(link -> allocateLinkResource(tx, link, allocations));
296 tx.commit(); 295 tx.commit();
297 - } catch (TransactionException | ResourceAllocationException e) { 296 + } catch (ResourceAllocationException e) {
298 log.error("Exception thrown, rolling back", e); 297 log.error("Exception thrown, rolling back", e);
299 tx.abort(); 298 tx.abort();
300 } catch (Exception e) { 299 } catch (Exception e) {
...@@ -407,12 +406,8 @@ public class ConsistentLinkResourceStore extends ...@@ -407,12 +406,8 @@ public class ConsistentLinkResourceStore extends
407 after.remove(allocations); 406 after.remove(allocations);
408 linkAllocs.replace(linkId, before, after); 407 linkAllocs.replace(linkId, before, after);
409 }); 408 });
410 - tx.commit(); 409 + success = tx.commit();
411 - success = true; 410 + } catch (Exception e) {
412 - } catch (TransactionException e) {
413 - log.debug("Transaction failed, retrying", e);
414 - tx.abort();
415 - } catch (Exception e) {
416 log.error("Exception thrown during releaseResource {}", allocations, e); 411 log.error("Exception thrown during releaseResource {}", allocations, e);
417 tx.abort(); 412 tx.abort();
418 throw e; 413 throw e;
......