Skip to content

Conversation

facuMH
Copy link
Contributor

@facuMH facuMH commented Jul 20, 2022

The idea behind this is to able to simulate a run to analyze behavior and how would celerity scale to larger amounts nodes, without necessarily having those nodes.
For this we are not flushing commands unless they are epochs ( and this ones only to the master node).

Copy link
Member

@psalz psalz left a comment

Choose a reason for hiding this comment

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

Thanks! I've added some notes. Also, I'm wondering whether we should call the variable CELERITY_DRY_RUN_NODES instead? 🤔

Copy link
Contributor

@PeterTh PeterTh left a comment

Choose a reason for hiding this comment

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

Looks pretty good, as long as you take care of the comments. Also, DON'T SCREAM AT THE USER :P

@facuMH facuMH force-pushed the dry_run branch 3 times, most recently from 333c2ef to 56276cf Compare July 26, 2022 11:17
Copy link
Member

@psalz psalz left a comment

Choose a reason for hiding this comment

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

I've added a few more notes. Also I vote in favor of renaming the env var to CELERITY_DRY_RUN_NODES - thoughts?

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

Copy link

@BlackMark29A BlackMark29A left a comment

Choose a reason for hiding this comment

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

I've identified some spots where readability/documentation could be improvied, but there are also some bugs which crept in:

@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

Copy link

@BlackMark29A BlackMark29A left a comment

Choose a reason for hiding this comment

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

Thank you. Looks good to me now.

Copy link
Contributor

@fknorr fknorr left a comment

Choose a reason for hiding this comment

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

LGTM up to the error message, thanks!

@github-actions
Copy link

github-actions bot commented Aug 2, 2022

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Member

@psalz psalz left a comment

Choose a reason for hiding this comment

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

LGTM!

@psalz psalz merged commit 299ebbf into master Sep 7, 2022
@psalz psalz deleted the dry_run branch September 7, 2022 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants