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

minor cleanup in job-manager journal and job-list (mostly inline docs) #5850

Merged
merged 5 commits into from Apr 5, 2024

Conversation

garlick
Copy link
Member

@garlick garlick commented Apr 2, 2024

@chu11 and I were both looking through the job-list source to be sure all is OK before we tag, and we both spotted some incorrect source comments. Al, feel free to add to this PR.

Copy link
Member

@chu11 chu11 left a comment

Choose a reason for hiding this comment

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

LGTM, per offline discussion, my stuff will go into a different PR.

@garlick garlick changed the title job-list: minor cleanup of inline doc minor cleanup in job-manager journal and job-list (mostly inline docs) Apr 2, 2024
@garlick
Copy link
Member Author

garlick commented Apr 3, 2024

I added some more - maybe have one more look before I set MWP?

@chu11
Copy link
Member

chu11 commented Apr 3, 2024

LGTM still

@garlick
Copy link
Member Author

garlick commented Apr 5, 2024

restarting fedora 38 builder that failed here:

2024-04-03T14:36:57.2543908Z not ok 7 - flux-cron tab works for month
2024-04-03T14:36:57.2544186Z 
2024-04-03T14:36:57.2544369Z expecting success: 
2024-04-03T14:36:57.2544670Z     $set_faketime Jun 5 15:45 2016 &&
2024-04-03T14:36:57.2545175Z     id=$(flux_cron at "Jun 6 15:45:00 2016" flux event pub t.cron.complete) &&
2024-04-03T14:36:57.2545723Z     test_when_finished "flux cron delete ${id}" &&
2024-04-03T14:36:57.2546217Z     next=$(date +%s --date="Jun 6 15:45 2016") &&
2024-04-03T14:36:57.2546629Z     cron_entry_check ${id} type datetime &&
2024-04-03T14:36:57.2547029Z     cron_entry_check ${id} stopped false &&
2024-04-03T14:36:57.2547473Z     cron_entry_check ${id} typedata.next_wakeup ${next} &&
2024-04-03T14:36:57.2547927Z     ${event_trace} t.cron t.cron.complete \
2024-04-03T14:36:57.2548318Z         $set_faketime Jun 6 15:45 2016 &&
2024-04-03T14:36:57.2548712Z     cron_entry_check ${id} stats.count 1 &&
2024-04-03T14:36:57.2549096Z     cron_entry_check ${id} stopped true
2024-04-03T14:36:57.2549364Z 
2024-04-03T14:36:57.2549684Z libfaketime: In parse_ft_string(), failed to parse FAKETIME timestamp.
2024-04-03T14:36:57.2550283Z Please check specification  with format %Y-%m-%d %T

Problem: a comment in job-list claims the flux-restart event will
not be seen, but recent changes have made it possible when processing
the journal backlog, if the system was shut down with jobs in SCHED
state.

Update comment.
Problem: job-manager defines DEFAULT_JOURNAL_SIZE_LIMIT but
this is no longer used.

Drop definition.
Problem: some commented out code uses EVENT_JOURNAL_ONLY
but this flag was renamed to EVENT_NO_COMMIT.

Rename flag.
Problem: a comment refers to event sequence numbers but
we don't have those anymore.

Update comment.
Problem: the journal protocol is undocumented.

Add a comment block at the top of journal.c that'll do for now.
@mergify mergify bot merged commit 3a79643 into flux-framework:master Apr 5, 2024
33 checks passed
Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Merging #5850 (b47b3c9) into master (4598d41) will increase coverage by 0.03%.
Report is 6 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5850      +/-   ##
==========================================
+ Coverage   83.26%   83.30%   +0.03%     
==========================================
  Files         513      513              
  Lines       82727    82727              
==========================================
+ Hits        68882    68915      +33     
+ Misses      13845    13812      -33     
Files Coverage Δ
src/modules/job-list/job_state.c 75.82% <ø> (ø)
src/modules/job-manager/journal.c 80.34% <ø> (ø)
src/modules/job-manager/prioritize.c 72.61% <ø> (ø)

... and 18 files with indirect coverage changes

@garlick garlick deleted the job_list_cleanup branch April 9, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants