stefan-egli commented on PR #12: URL: https://github.com/apache/sling-org-apache-sling-discovery-base/pull/12#issuecomment-2812673383
The reason I asked is that the interplay between `bindSystemReadyService`, `unbindSystemReadyService`, `activate` and `deactivate` with how they set the "readiness state" is not clear to me. Essentially I don't think the way it is done now works as intended. For example `unbindSystemReadyService` assumes `deactivate` is called before - but that's not how I understood this works. `unbindSystemReadyService` I assume is called as soon as the system is no longer ready. but that can be a long time before actual `deactivate` is called. (similar with `bindSystemReadyService` where it only calls `setSystemReady(true)` in a very specific case, not covering all possibilities). * It seems the main introduced hook here is `shouldDelayTopologyChange` which is invoked in `getTopology`. This seems appropriate for startup (as the "delay" aspect is from detached to being attached). For shutdown I fail to see how this would have any effect. At the current state of the draft shutdown doesn't seem covered - I think what it should do is cause a TOPOLOGY_CHANGING and stay there (but I don't see that happening from the current code - it should invoke `oldView.setNotCurrent()` somewhere...). * In that context : `TopologyDelayHandler` doesn't seem to capture well what the class should be used for. I think it's not so much about delay but about system readiness. * IIUC then the idea of `startupTimeout` is to "force mark the instance as ready" after the timeout. That is a bit unexpected from the naming - perhaps having that explicitly described in some comment would be good. But besides that : do we want this? * I think the `startupInProgress`, `systemReady` and `shutdownInProgress` being 3 different AtomicBooleans is making the code somewhat complex. It might be easier to track one state - as the state transitions are then easier to catch. * How should it behave when `startupTimeout==0` : it seems it sets `startupInProgress.set(false)`, but then nothing further ever happens. It will never set `systemReady` to `true`. Which is unexpected. * What I'm also wondering is : should `systemReadyService==null` be supported. It seems the `systemReadyService` is essential to `TopologyDelayHandler` and the way it is implemented now it seems to bypass it in certain cases. Which I'm not sure helps... * The `SystemReadyService` would probably be good to extend, as it is currently only an interface that leaves out a few questions: for example : what is the idea of `isSystemReady`, when is that true, when false. Isn't the idea of the `SystemReadyService` that is is only registered when the system is ready (how else would depending services be able to know when `isSystemReady` switches between false and true. I think it doesn't need a `isSystemReady`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
