Skip to content
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

User session status split #365

Merged
merged 10 commits into from
May 27, 2022
Merged

User session status split #365

merged 10 commits into from
May 27, 2022

Conversation

bboudaoud-nv
Copy link
Collaborator

@bboudaoud-nv bboudaoud-nv commented Apr 18, 2022

This branch splits the per-session written user status (sessions completed) from the remainder of the user status file. The new per-session status file format supports an iterative write approach and therefore serializes to disk faster. This file uses a CSV format with user ID, session ID formatting for each line. It is read in once from disk at startup to recover the user session status for all users.

The old user status file still contains the currentUser field (and potentially adding new users) which requires write-back when the user changes/is added but otherwise should be written far less frequently.

Custom session status log names can be specified using the new completedLogFilename configuration parameter in the top-level of the user status file.

Merging this branch closes #355

@bboudaoud-nv bboudaoud-nv added the enhancement New feature or request label Apr 18, 2022
@bboudaoud-nv bboudaoud-nv requested a review from jspjutNV April 18, 2022 14:35
@bboudaoud-nv bboudaoud-nv self-assigned this Apr 18, 2022
@bboudaoud-nv bboudaoud-nv changed the title User status split User session status split Apr 18, 2022
@bboudaoud-nv bboudaoud-nv marked this pull request as ready for review May 2, 2022 19:43
@bboudaoud-nv
Copy link
Collaborator Author

This might interact w/ #348, perhaps we should attempt a fix for that issue in this branch

@jspjutNV jspjutNV added this to the Next Release (mid 2022) milestone May 25, 2022
@jspjutNV
Copy link
Contributor

I checked out this branch and it fails to build. I'm not sure what's going on yet, but the error happens outside of the code from this PR, but doesn't happen on the master branch. I'm investigating.

@jspjutNV
Copy link
Contributor

73f8b05 fixed the build error.

@jspjutNV
Copy link
Contributor

There appears to be a remaining bug, or at least weird behavior if allowRepeat is on, and the user selects sessions out of order. We sort of cover this in the readme, and allowRepeat is intended to be only used when the user can't select session order, so I think we can consider this documented behavior. I'll clarify a little in the docs.

Copy link
Contributor

@jspjutNV jspjutNV left a comment

Choose a reason for hiding this comment

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

I made some changes to docs and fixed the test case that was no longer testing everything. This appears to work in my tests, but we'll want to be aware that something may break and need fixing. This does break the current "create user" hack somewhat used by some experiments, so we will want to prioritize that fix for the next release.

@jspjutNV jspjutNV merged commit 778282c into master May 27, 2022
@jspjutNV jspjutNV deleted the UserStatusSplit branch May 27, 2022 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance degredation when user status has a large number of users
2 participants