-
Notifications
You must be signed in to change notification settings - Fork 303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add LogRecord table update callbacks #2159
base: master
Are you sure you want to change the base?
Conversation
npostnikova
commented
Jun 13, 2022
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this check? And maybe the super call too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove it? Sending logs doesn't make too much sense if we just have reset the state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.