job-info: misc cleanup #5586
Merged
job-info: misc cleanup #5586
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Unlikely. That one only touches the |
garlick
approved these changes
Nov 22, 2023
src/modules/job-info/update.c
Outdated
| cmpmsg = flux_msglist_next (uc->msglist); | ||
| } | ||
| if (cancel) | ||
| flux_msglist_cancel (uc->ctx->h, uc->msglist, msg); |
There was a problem hiding this comment.
flux_msglist_cancel() returns -1 if the respond fails so probably that should be logged, like above only ideally with a bit better error message.
Problem: After a failed call to strdup(), code sets errno to ENOMEM. This isn't necessary because strdup() does this already. Remove the manual setting of errno to ENOMEM.
Problem: A call to json_pack_ex() + json_dumps() + flux_msg_set_string() can be simplified via flux_msg_vpack(). Simplify code and just call flux_msg_vpack().
Problem: Code for dealing with update watch cancellations / disconnects can be greatly simplified by using the flux_msglist_cancel() and flux_msglist_disconnect(). In addition, ENODATA should only be returned on cancels, not disconnects. Simplify code by using flux_msglist_cancel() and flux_msglist_disconnect(). This cleanup also corrects the error where ENODATA is returned on disconnects.
Problem: Some code in job-info's update-watch code uses internal eventlog parsing functions that serve no benefit. Use libeventlog functions to parse the eventlog, thus cleaning up the code.
Problem: Several eventlog utility functions in job-info are prefixed with "eventlog_". By using this naming convention, readers may think these functions are from the libeventlog library. Rename the functions to remove the "eventlog_" prefix.
Problem: In the job-info guest-watch code, the bool value "cancel" and send_cancel() function are a little ambiguous in what they do. Rename the variable "cancel" to "eventlog_watch_canceled" and rename the function send_cancel() to send_eventlog_watch_cancel().
Problem: In the job-info watch code, the bool value "cancel" and watch_cancel() function are a little ambiguous in what they do. Rename the variable "cancel" to "kvs_watch_canceled" and rename the function watch_cancel() to send_kvs_watch_cancel().
Problem: In guest-watch and watch code, ENODATA is sent to the original caller both when the stream is canceled and when it is disconnected. It should not be sent when it is disconnected. Add an internal flag to only send ENODATA when the watch stream is canceled, not when it is disconnected.
Problem: A function definition exceeded 80 characters, which violates RFC7. Fix the code style.
|
@garlick thanks! Fixed that and I also noticed a line > 80 chars near that, so I added that in a dumb fix at the top of this commit series. Setting MWP |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #5586 +/- ##
==========================================
- Coverage 83.45% 83.41% -0.05%
==========================================
Files 486 486
Lines 82450 82456 +6
==========================================
- Hits 68810 68781 -29
- Misses 13640 13675 +35
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Misc cleanup, mostly coming from comments in #5467
Note that I just saw #5585 got submitted, dunno if there are conflicts ...