CELDEV-1314 - Outgoing/IncomingObservationManager#289
Conversation
fpichler
changed the title
fpichler
left a comment
fpichler
left a comment
There was a problem hiding this comment.
The removal of the LGPL header from IncomingObservationManager.java (and its omission from the newly created OutgoingObservationManager.java) is problematic.
Could we please restore the LGPL headers to the modified files and add them to the newly created files? According to the Free Software Foundation's instructions on applying GNU licenses, a root LICENSE file isn't sufficient on its own. They state: 'You also need to put a license notice in each file of the program... because anyone could copy an individual file out of the package.'
| checkState(networkAdapter != null, | ||
| "remote observation is enabled but network adapter implementation missing"); |
There was a problem hiding this comment.
In edge cases, when the NetworkAdapter bean disappears at runtime (e.g., during Spring shutdown), the checkState will throw an unchecked IllegalStateException inside processQueue(), that exception is not caught. It bubbles up and out of the finally block, and out of ObservationManager.notify().
Why this matters: The code that triggered the local event (for example, a user saving a document, an HTTP request handler, or a background job) will suddenly receive an unexpected IllegalStateException bubbling up from the observation layer.
- If this happens during an HTTP request, the user gets a 500 Internal Server Error.
- If it happens during a database transaction, it might cause a transaction rollback.
Suggestion: Consider moving the checkState inside the try-catch block (or handle it in processQueue()), the system will simply log the ERROR ("network adapter missing") and gracefully return. The local thread that initiated the event will continue completely undisturbed.
There was a problem hiding this comment.
Dropping "Remote" from the name makes IncomingObservationManager and OutgoingObservationManager sound like generic local event routers.
Rename to IncomingRemoteObservationManager (or RemoteEventReceiver). This immediately signals that it handles network-bound events, avoiding confusion with the core ObservationManager.
There was a problem hiding this comment.
Dropping "Remote" from the name makes IncomingObservationManager and OutgoingObservationManager sound like generic local event routers.
Rename to OutgoingRemoteObservationManager (or RemoteEventDispatcher). This immediately signals that it handles network-bound events, avoiding confusion with the core ObservationManager.
| return configuration.isEnabled() && !remoteEventManagerContext.isRemoteState(); | ||
| } | ||
|
|
||
| public void notifyLocalThenRemote( |
There was a problem hiding this comment.
notifyLocalThenRemote(LocalEventData, Consumer): While it explains exactly what happens, it reads more like a comment than a method name.
Alternative: notifyWithRemotePropagation(...) or executeWithRemoteFlush(...).
| } | ||
| } | ||
|
|
||
| private boolean queue(LocalEventData localEvent) { |
There was a problem hiding this comment.
queue(LocalEventData): This method name is a bit ambiguous (noun or verb?) and it returns a boolean representing whether this is the first event in the queue.
Alternative: enqueueAndCheckIfFirst(...) or initQueueAndAdd(...) which makes the boolean return value obvious.
| private final EventConverterManager eventConverterManager; | ||
| private final RemoteObservationManagerContext remoteEventManagerContext; | ||
| private final BeanFactory beanFactory; | ||
| private final ThreadLocal<LinkedList<LocalEventData>> eventQueue = new ThreadLocal<>(); |
There was a problem hiding this comment.
eventQueue (ThreadLocal<LinkedList<LocalEventData>>): Naming a ThreadLocal just eventQueue hides its thread-bound nature when reading the code further down.
Alternative: pendingRemoteEvents or threadLocalEventQueue.
2 participants
https://synjira.atlassian.net/browse/CELDEV-1314