Add LogRecord table update callbacks #2159
Conversation
npostnikova
commented
Jun 13, 2022
npostnikova
commented
- Trigger txn sending to Colloboque when a new log record is inserted
- Send txns sequentially
| protected Unit onProjectLogUpdate() { | ||
| if (isColloboqueLocalTest()) { | ||
| super.onProjectLogUpdate(); | ||
| if (!isSendingInProgress.get()) sendProjectStateLogs(); |
There was a problem hiding this comment.
The call is guarded with the flag check here, but not guarded a few lines below. I suggest moving the check of isSendingInProgress directly into sendProjectStateLogs
There was a problem hiding this comment.
It was designed this weird because fireXlogReceived & onBaseTxnIdReceived reset the flag
| } | ||
| return Unit.INSTANCE; | ||
| } | ||
|
|
||
| private Unit fireXlogReceived(ServerCommitResponse response) { | ||
| myBaseTxnCommitInfo.update(response.getBaseTxnId(), response.getNewBaseTxnId(), 1); | ||
| isSendingInProgress.set(false); |
There was a problem hiding this comment.
I suggest replacing this boolean flag and long-living success response listener with a short-living listener, working as follows:
- Instantiate it and assign to some AtomicReference in
setProjectStateLogs - Short-living listener is not null <=> isSendingInProgress
- Clear the reference in the listener itself, and call fireXlogReceived
Pros:
- All operations with this lock will be textually scoped in one function
- You will be able to capture the communication state, e.g. the timestamp when the request was sent, and display the progress or report timeout, etc
There was a problem hiding this comment.
Thank you so much for the idea! I hated this AtomicBoolean but I couldn't come up with something better
Not sure if I implemented this exactly as expected though...
ganttproject/src/main/java/net/sourceforge/ganttproject/GanttProject.java
Outdated
Show resolved
Hide resolved
| var listener = txnSendingListener.get(); | ||
| // Websocket is [re-]started. Previous messages are discarded. | ||
| if (listener != null) listener.onSendCompleted(); | ||
| sendProjectStateLogs(); |
There was a problem hiding this comment.
It appears that the comment above (previous messages are discarded) and these two lines (sendCompleted, sendLogs) are kinda controversial. We obviously need to make sure that the state which we have locally matches the server state which corresponds to this baseTxnId. Until it is not done, we probably want to really discard the changes, that is, clear the state, whatever it is, but not do what we do in the normal success case.
There was a problem hiding this comment.
Thanks!
Added a TODO for performing synchronization with the server. For now, logs are sent only when the server specifies that the project is empty.
| } | ||
| } catch (ProjectDatabaseException e) { | ||
| gpLogger.error("Failed to send logs", new Object[]{}, ImmutableMap.of(), e); | ||
| } | ||
| return Unit.INSTANCE; | ||
| } | ||
|
|
||
| @Override | ||
| protected Unit onProjectLogUpdate() { | ||
| if (isColloboqueLocalTest()) { |
There was a problem hiding this comment.
I don't like that we have more and more these checks. I thought that we would have just a few differences between the "local test" and "prod" modes (url, authentication), but now it seems that we will just not do anything in prod.
There was a problem hiding this comment.
remove this check? And maybe the super call too?
There was a problem hiding this comment.
Hmmm
We perform websocket handshake (with base txn id & project refid exchange) only under this check.
If we remove the check, we'll log an error every time a new log record is added for an offline document, won't we?
There was a problem hiding this comment.
Well, in this case we shall check "if we are working with an online document, in any sense", not "is it a local online document setup".
We probably need to modify the local dev server so that it could respond to project read and write operations, just like a regular GP Cloud server. In this case the check may look like if (project.document is OnlineDocument)
ganttproject/src/main/java/net/sourceforge/ganttproject/GanttProject.java
Show resolved
Hide resolved
ganttproject/src/main/java/net/sourceforge/ganttproject/GanttProject.java
Outdated
Show resolved
Hide resolved
| } | ||
| } catch (ProjectDatabaseException e) { | ||
| gpLogger.error("Failed to send logs", new Object[]{}, ImmutableMap.of(), e); | ||
| } | ||
| return Unit.INSTANCE; | ||
| } | ||
|
|
||
| @Override | ||
| protected Unit onProjectLogUpdate() { | ||
| if (isColloboqueLocalTest()) { |
There was a problem hiding this comment.
remove this check? And maybe the super call too?
| if (EMPTY_LOG_BASE_TXN_ID.equals(baseTxnId)) { | ||
| myBaseTxnCommitInfo.reset(); | ||
| myBaseTxnCommitInfo.update(null, baseTxnId, 0); | ||
| sendProjectStateLogs(); |
There was a problem hiding this comment.
remove it? Sending logs doesn't make too much sense if we just have reset the state.
There was a problem hiding this comment.
In such a case, we'll send all the logs from the beginning, one by one
Where should we start sending logs instead? I thought that it's the right place
There was a problem hiding this comment.
Send the logs when an undoable edit happens. But I don't think that a user has any chance to make an undoable edit immediately at the moment when we reset the log.
