Best practices for a few scheduler functions #3779
Open
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
…_time() Move from libpe_status to libcrmcommon, rename to pcmk__scheduler_epoch_time(), add a doxygen block, and improve log messages and formatting.
We currently have pe_reset_working_data(), cleanup_calculations(), and set_working_set_defaults() for partially freeing and restoring defaults in scheduler data. The distinction is confusing, and this is the first step to rationalizing it: the new function sets nonzero default values in scheduler data, without freeing or nulling/zeroing anything, so it can be used in both existing and replacement functions.
... from libpe_status to libcrmcommon. Also rename with a pcmk prefix, add doxygen blocks, and improve variable names. There are no unit tests because the relevant struct is source-file-scoped, and the APIs aren't complex enough to make it worth exposing.
... as a convenience wrapper around the resource free() method, suitable for use as a GList free function.
... in libpe_status with a new pcmk__free_node() in libcrmcommon that can be used as a GList iterator.
... in libpe_status with a new pcmk__free_action_relation() in libcrmcommon that can be used as a GList iterator
... in libpe_status with a new pcmk__free_location() in libcrmcommon that can be used as a GList iterator
... in libpe_status with a new pcmk__free_action() in libcrmcommon that can be used as a GList iterator
This is the equivalent of pe_reset_working_set(), except for freeing and zeroing members individually rather than zeroing the whole object.
It is fully equivalent to pcmk_reset_scheduler()
cleanup_calculations() is equivalent to pcmk_reset_scheduler() except for not freeing ordering, location, or colocation constraints. However it logged an assertion if location or ordering constraints were non-NULL, and it called set_working_set_defaults() which would NULL the constraint members without freeing them. Therefore we can simply replace calls with pcmk_reset_scheduler(). There's no reason to ever zero constraints without freeing them.
The function zeroed out the scheduler object without freeing members, which makes no sense, so simply replace calls with pcmk_reset_scheduler(), which does a free-and-zero.
... and use it internally instead of pe_new_working_set()
... and use it internally instead of pe_free_working_set()
... aside from deprecated API names
... as pcmk__update_recheck_time(). The only code changes are the name and checking for NULL arguments.
|
@nrwahl2 , here's my last best practices PR for review when you get a chance |
|
:'( |
nrwahl2
reviewed
Dec 24, 2024
| #if PCMK__CONCURRENT_FENCING_DEFAULT_TRUE | ||
| pcmk__set_scheduler_flags(scheduler, | ||
| pcmk__sched_symmetric_cluster | ||
| |pcmk__sched_concurrent_fencing |
There was a problem hiding this comment.
could just guard this one line, unless you think this is more readable
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.
No description provided.