Skip to content

[Backend] Adjust active runtime initialization logic #495

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

[Backend] Adjust active runtime initialization logic #495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
98ebd07
Add JWT_SECRET in spawner
Aug 30, 2023
98be7a0
Adjust active runtime init logic
Aug 30, 2023
baec139
Merge remote-tracking branch 'refs/remotes/upstream/main' into pr_493…
Aug 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions apps/spawner/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,7 @@ export async function startAPIServer({ port }) {
const doc = getMyYDoc({ repoId, token });
const rootMap = doc.getMap("rootMap");
const runtimeMap = rootMap.get("runtimeMap") as Y.Map<RuntimeInfo>;
runtimeMap.set(runtimeId, {});
// runtimeMap.delete(runtimeId)
runtimeMap.delete(runtimeId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good change, I meant to use runtimeMap.delete but forgot to uncomment it. I've actually included this in my local code.

return true;
},

Expand Down
12 changes: 10 additions & 2 deletions apps/ui/src/lib/store/yjsSlice.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,10 +197,18 @@ export const createYjsSlice: StateCreator<MyState, [], [], YjsSlice> = (
});
}
);
// Set active runtime to the first one.
// Set active runtime
const runtimeMap = get().getRuntimeMap();
if (runtimeMap.size > 0) {
get().setActiveRuntime(Array.from(runtimeMap.keys())[0]);
const runtimeList = Array.from(runtimeMap.keys());
const activeInstances = runtimeList.filter((id) => {
return runtimeMap.get(id)?.status !== undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The status must === "connected" to be alive. It could be undefined, "disconnected", or "connecting".

But I'd suggest keeping the original logic (use the first runtime regardless of its status) for now.

There are many remaining logic to add:

  1. This assignment only happens at the initial page loading. When there's no runtime, creating a new runtime wouldn't activate it. So we also need to automatically activate the runtime when creating a new runtime (if there's no runtime).
  2. We also need to create runtime automatically when creating or opening a repo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will continue to work on the runtime-related staff. You can focus on the selective view and don't worry too much at the runtime.

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 noticed that the active runtime continues reset in the UI refactor PR, and realized the root cause here. I will close this PR then.

Copy link
Collaborator

@lihebi lihebi Aug 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

active runtime continues reset

The runtime isn't stable, but this isn't the root cause, and fixing this alone couldn't make it stable.

});
if (activeInstances) {
get().setActiveRuntime(activeInstances[0]);
} else {
get().setActiveRuntime(runtimeList[0]);
}
}
// Set up observers to trigger future runtime status changes.
runtimeMap.observe(
Expand Down