Committed by
Gerrit Code Review
Added a temporary fix for out-of-order app activation event delivery.
Cleaning up app subsystem code a tiny bit as well. Change-Id: I5df7d4c6d62d122653331474fb079648e779d595
Showing
2 changed files
with
16 additions
and
13 deletions
| ... | @@ -80,8 +80,6 @@ public class ApplicationManager | ... | @@ -80,8 +80,6 @@ public class ApplicationManager |
| 80 | @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY) | 80 | @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY) |
| 81 | protected FeaturesService featuresService; | 81 | protected FeaturesService featuresService; |
| 82 | 82 | ||
| 83 | - private boolean initializing; | ||
| 84 | - | ||
| 85 | // Application supplied hooks for pre-activation processing. | 83 | // Application supplied hooks for pre-activation processing. |
| 86 | private final Multimap<String, Runnable> deactivateHooks = HashMultimap.create(); | 84 | private final Multimap<String, Runnable> deactivateHooks = HashMultimap.create(); |
| 87 | private final Cache<ApplicationId, CountDownLatch> pendingOperations = | 85 | private final Cache<ApplicationId, CountDownLatch> pendingOperations = |
| ... | @@ -92,11 +90,7 @@ public class ApplicationManager | ... | @@ -92,11 +90,7 @@ public class ApplicationManager |
| 92 | @Activate | 90 | @Activate |
| 93 | public void activate() { | 91 | public void activate() { |
| 94 | eventDispatcher.addSink(ApplicationEvent.class, listenerRegistry); | 92 | eventDispatcher.addSink(ApplicationEvent.class, listenerRegistry); |
| 95 | - | ||
| 96 | - initializing = true; | ||
| 97 | store.setDelegate(delegate); | 93 | store.setDelegate(delegate); |
| 98 | - initializing = false; | ||
| 99 | - | ||
| 100 | log.info("Started"); | 94 | log.info("Started"); |
| 101 | } | 95 | } |
| 102 | 96 | ... | ... |
| ... | @@ -30,6 +30,7 @@ import org.apache.felix.scr.annotations.Deactivate; | ... | @@ -30,6 +30,7 @@ import org.apache.felix.scr.annotations.Deactivate; |
| 30 | import org.apache.felix.scr.annotations.Reference; | 30 | import org.apache.felix.scr.annotations.Reference; |
| 31 | import org.apache.felix.scr.annotations.ReferenceCardinality; | 31 | import org.apache.felix.scr.annotations.ReferenceCardinality; |
| 32 | import org.apache.felix.scr.annotations.Service; | 32 | import org.apache.felix.scr.annotations.Service; |
| 33 | +import org.onlab.util.Tools; | ||
| 33 | import org.onosproject.app.ApplicationDescription; | 34 | import org.onosproject.app.ApplicationDescription; |
| 34 | import org.onosproject.app.ApplicationEvent; | 35 | import org.onosproject.app.ApplicationEvent; |
| 35 | import org.onosproject.app.ApplicationException; | 36 | import org.onosproject.app.ApplicationException; |
| ... | @@ -89,11 +90,14 @@ import static org.slf4j.LoggerFactory.getLogger; | ... | @@ -89,11 +90,14 @@ import static org.slf4j.LoggerFactory.getLogger; |
| 89 | * Manages inventory of applications in a distributed data store providing | 90 | * Manages inventory of applications in a distributed data store providing |
| 90 | * stronger consistency guarantees. | 91 | * stronger consistency guarantees. |
| 91 | */ | 92 | */ |
| 92 | -@Component(immediate = true, enabled = true) | 93 | +@Component(immediate = true) |
| 93 | @Service | 94 | @Service |
| 94 | public class DistributedApplicationStore extends ApplicationArchive | 95 | public class DistributedApplicationStore extends ApplicationArchive |
| 95 | implements ApplicationStore { | 96 | implements ApplicationStore { |
| 96 | 97 | ||
| 98 | + // FIXME: eliminate the need for this | ||
| 99 | + private static final int FIXME_ACTIVATION_DELAY = 500; | ||
| 100 | + | ||
| 97 | private final Logger log = getLogger(getClass()); | 101 | private final Logger log = getLogger(getClass()); |
| 98 | 102 | ||
| 99 | private static final MessageSubject APP_BITS_REQUEST = new MessageSubject("app-bits-request"); | 103 | private static final MessageSubject APP_BITS_REQUEST = new MessageSubject("app-bits-request"); |
| ... | @@ -144,7 +148,7 @@ public class DistributedApplicationStore extends ApplicationArchive | ... | @@ -144,7 +148,7 @@ public class DistributedApplicationStore extends ApplicationArchive |
| 144 | public void activate() { | 148 | public void activate() { |
| 145 | messageHandlingExecutor = Executors.newSingleThreadExecutor( | 149 | messageHandlingExecutor = Executors.newSingleThreadExecutor( |
| 146 | groupedThreads("onos/store/app", "message-handler", log)); | 150 | groupedThreads("onos/store/app", "message-handler", log)); |
| 147 | - clusterCommunicator.<String, byte[]>addSubscriber(APP_BITS_REQUEST, | 151 | + clusterCommunicator.addSubscriber(APP_BITS_REQUEST, |
| 148 | bytes -> new String(bytes, Charsets.UTF_8), | 152 | bytes -> new String(bytes, Charsets.UTF_8), |
| 149 | name -> { | 153 | name -> { |
| 150 | try { | 154 | try { |
| ... | @@ -180,7 +184,7 @@ public class DistributedApplicationStore extends ApplicationArchive | ... | @@ -180,7 +184,7 @@ public class DistributedApplicationStore extends ApplicationArchive |
| 180 | * Processes existing applications from the distributed map. This is done to | 184 | * Processes existing applications from the distributed map. This is done to |
| 181 | * account for events that this instance may be have missed due to a staggered start. | 185 | * account for events that this instance may be have missed due to a staggered start. |
| 182 | */ | 186 | */ |
| 183 | - void bootstrapExistingApplications() { | 187 | + private void bootstrapExistingApplications() { |
| 184 | apps.asJavaMap().forEach((appId, holder) -> setupApplicationAndNotify(appId, holder.app(), holder.state())); | 188 | apps.asJavaMap().forEach((appId, holder) -> setupApplicationAndNotify(appId, holder.app(), holder.state())); |
| 185 | 189 | ||
| 186 | } | 190 | } |
| ... | @@ -253,7 +257,7 @@ public class DistributedApplicationStore extends ApplicationArchive | ... | @@ -253,7 +257,7 @@ public class DistributedApplicationStore extends ApplicationArchive |
| 253 | public void setDelegate(ApplicationStoreDelegate delegate) { | 257 | public void setDelegate(ApplicationStoreDelegate delegate) { |
| 254 | super.setDelegate(delegate); | 258 | super.setDelegate(delegate); |
| 255 | executor.execute(this::bootstrapExistingApplications); | 259 | executor.execute(this::bootstrapExistingApplications); |
| 256 | - executor.schedule(() -> loadFromDisk(), APP_LOAD_DELAY_MS, TimeUnit.MILLISECONDS); | 260 | + executor.schedule((Runnable) this::loadFromDisk, APP_LOAD_DELAY_MS, TimeUnit.MILLISECONDS); |
| 257 | } | 261 | } |
| 258 | 262 | ||
| 259 | @Override | 263 | @Override |
| ... | @@ -296,7 +300,7 @@ public class DistributedApplicationStore extends ApplicationArchive | ... | @@ -296,7 +300,7 @@ public class DistributedApplicationStore extends ApplicationArchive |
| 296 | } | 300 | } |
| 297 | 301 | ||
| 298 | private boolean hasPrerequisites(ApplicationDescription app) { | 302 | private boolean hasPrerequisites(ApplicationDescription app) { |
| 299 | - return !app.requiredApps().stream().map(n -> getId(n)) | 303 | + return !app.requiredApps().stream().map(this::getId) |
| 300 | .anyMatch(id -> id == null || getApplication(id) == null); | 304 | .anyMatch(id -> id == null || getApplication(id) == null); |
| 301 | } | 305 | } |
| 302 | 306 | ||
| ... | @@ -342,6 +346,11 @@ public class DistributedApplicationStore extends ApplicationArchive | ... | @@ -342,6 +346,11 @@ public class DistributedApplicationStore extends ApplicationArchive |
| 342 | } | 346 | } |
| 343 | activateRequiredApps(vAppHolder.value().app()); | 347 | activateRequiredApps(vAppHolder.value().app()); |
| 344 | 348 | ||
| 349 | + // FIXME: Take a breath before the post-order operation to allow required app | ||
| 350 | + // activation events to fully propagate. There appears to be an out-of-order | ||
| 351 | + // event delivery issue that needs to be fixed. | ||
| 352 | + Tools.delay(FIXME_ACTIVATION_DELAY); | ||
| 353 | + | ||
| 345 | apps.computeIf(appId, v -> v != null && v.state() != ACTIVATED, | 354 | apps.computeIf(appId, v -> v != null && v.state() != ACTIVATED, |
| 346 | (k, v) -> new InternalApplicationHolder( | 355 | (k, v) -> new InternalApplicationHolder( |
| 347 | v.app(), ACTIVATED, v.permissions())); | 356 | v.app(), ACTIVATED, v.permissions())); |
| ... | @@ -557,7 +566,7 @@ public class DistributedApplicationStore extends ApplicationArchive | ... | @@ -557,7 +566,7 @@ public class DistributedApplicationStore extends ApplicationArchive |
| 557 | /** | 566 | /** |
| 558 | * Internal class for holding app information. | 567 | * Internal class for holding app information. |
| 559 | */ | 568 | */ |
| 560 | - private static class InternalApplicationHolder { | 569 | + private static final class InternalApplicationHolder { |
| 561 | private final Application app; | 570 | private final Application app; |
| 562 | private final InternalState state; | 571 | private final InternalState state; |
| 563 | private final Set<Permission> permissions; | 572 | private final Set<Permission> permissions; |
| ... | @@ -569,7 +578,7 @@ public class DistributedApplicationStore extends ApplicationArchive | ... | @@ -569,7 +578,7 @@ public class DistributedApplicationStore extends ApplicationArchive |
| 569 | permissions = null; | 578 | permissions = null; |
| 570 | } | 579 | } |
| 571 | 580 | ||
| 572 | - public InternalApplicationHolder(Application app, InternalState state, Set<Permission> permissions) { | 581 | + private InternalApplicationHolder(Application app, InternalState state, Set<Permission> permissions) { |
| 573 | this.app = Preconditions.checkNotNull(app); | 582 | this.app = Preconditions.checkNotNull(app); |
| 574 | this.state = state; | 583 | this.state = state; |
| 575 | this.permissions = permissions == null ? null : ImmutableSet.copyOf(permissions); | 584 | this.permissions = permissions == null ? null : ImmutableSet.copyOf(permissions); | ... | ... |
-
Please register or login to post a comment