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

Dry run #125

Merged
merged 1 commit into from
Sep 7, 2022
Merged

Dry run #125

merged 1 commit into from
Sep 7, 2022

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? 🤔

src/runtime.cc Outdated Show resolved Hide resolved
src/config.cc Outdated Show resolved Hide resolved
src/config.cc Outdated Show resolved Hide resolved
include/config.h Outdated Show resolved Hide resolved
include/runtime.h Outdated Show resolved Hide resolved
test/runtime_tests.cc Outdated Show resolved Hide resolved
test/runtime_tests.cc Outdated Show resolved Hide resolved
test/runtime_tests.cc Outdated Show resolved Hide resolved
test/runtime_tests.cc Outdated Show resolved Hide resolved
src/config.cc Outdated Show resolved Hide resolved
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

include/config.h Outdated Show resolved Hide resolved
test/runtime_tests.cc Outdated Show resolved Hide resolved
test/test_utils.h Outdated Show resolved Hide resolved
test/test_utils.h Outdated Show resolved Hide resolved
test/runtime_tests.cc Outdated Show resolved Hide resolved
@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?

src/runtime.cc Outdated Show resolved Hide resolved
test/runtime_tests.cc Outdated Show resolved Hide resolved
test/test_utils.h Outdated Show resolved Hide resolved
test/test_utils.h Outdated Show resolved Hide resolved
test/test_utils.h Outdated Show resolved Hide resolved
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

include/runtime.h Outdated Show resolved Hide resolved
src/config.cc Outdated Show resolved Hide resolved
test/test_utils.h Outdated Show resolved Hide resolved
test/test_utils.h Outdated Show resolved Hide resolved
@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:

src/config.cc Outdated Show resolved Hide resolved
test/test_utils.h Outdated Show resolved Hide resolved
test/test_utils.h Outdated Show resolved Hide resolved
test/test_utils.h Outdated Show resolved Hide resolved
test/test_utils.h Outdated Show resolved Hide resolved
test/test_utils.h Outdated Show resolved Hide resolved
test/test_utils.h Outdated Show resolved Hide resolved
@github-actions
Copy link

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

test/runtime_tests.cc Outdated Show resolved Hide resolved
test/test_utils.h Outdated Show resolved Hide resolved
test/test_utils.h Outdated Show resolved Hide resolved
@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!

src/runtime.cc Outdated Show resolved Hide resolved
@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