Skip to content

[Backend] Reduce resultMap sync frequency #489

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

Merged
merged 3 commits into from
Aug 28, 2023

Conversation

senwang86
Copy link
Collaborator

@senwang86 senwang86 commented Aug 28, 2023

Summary

  • Reduce yjs sync via resultMap.set() method
  • Fix [Bug] resultPod. lastExecutedAt value is missing after execution. #483; very interesting solution from ChatGPT, while I didn't find its sources
    • Yjs is a CRDT framework that syncs JavaScript objects across multiple peers. However, it does not natively support all JavaScript types. The Date object is one of the types that Yjs does not natively support. When you try to sync a Date object, Yjs will only sync the reference to the object, not the actual date value. This is why you're seeing issues with syncing the "Date" field.

Test

Screenshot 2023-08-28 at 2 22 12 PM

@senwang86 senwang86 requested a review from lihebi August 28, 2023 21:40
let podId = payload["podId"] || undefined;
if (podId) {
const oldresult: PodResult = resultMap.get(podId) || { data: [] };
if (oldresult.last_exec_end) {
Copy link
Collaborator

@lihebi lihebi Aug 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think last_exec_end is unnecessary, and I intend to remove it. I was not setting it on purpose.

It was only used to clear the results. The result is now (better) cleared on the frontend right before executing the pod (code).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG, I missed the code here. Let me clean up the code.

@@ -78,7 +77,6 @@ export async function setupRuntimeSocket({
type: `${type}_${content.name}`,
text: content.text,
});
resultMap.set(podId, oldresult);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this unnecessary?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is necessary. Otherwise the stream won't be displayed until the very end. Try this:

import time
for i in range(5):
    print(i)
    time.sleep(1)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed the yjs sync resultMap at least 3 times during a single run, and intended to reduce the frequency. But from the example above, I agree that yjs should honer the kernel execution behavior even though the sync cost is high. Let me revert the change.

@lihebi
Copy link
Collaborator

lihebi commented Aug 28, 2023

Thanks!

@lihebi lihebi merged commit b7d6a49 into codepod-io:main Aug 28, 2023
@senwang86 senwang86 deleted the Fix_resultMap branch September 7, 2023 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] resultPod. lastExecutedAt value is missing after execution.
2 participants