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

Add completed / remaining bytes rebalance metrics #2133

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

HerveRiviere
Copy link

This PR resolves #2132

@mhratson mhratson mentioned this pull request Sep 5, 2024
1 task
mhratson added a commit that referenced this pull request Sep 6, 2024
## Summary
### Why
1. GIthub Actions workflow are native GH workflows 
2. Github Actions do not require additional non-github accounts unlike CircleCI
3. plenty of compute resources[^0] available for OSS projects
4. unlike CircleCI resource limits (don't have details)

[^0]:https://docs.github.com/en/actions/administering-github-actions/usage-limits-billing-and-administration#availability

### What
1. creates CI workflow `ci.yaml`
2. creates Artifactory workflow: `artifactory.yaml`

Workflow structure is documented in the spec[^1]

[^1]:https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions

## Expected Behavior
CI is expected to 
1. execute unit tests
1. execute integration tests
1. execute hw platform unit tests 
1. publish artifacts to the artifactory when a tag is published
1. provide ability to re-run tests on failures
1. report results to corresponding PR/branch 

which is to be used as quality gates for PR merging.

## Actual Behavior
1. current Circle CI integration provides [1] [2] [3] [4] from the expected behavior
4. but re-run-ing checks requires additional efforts like logging in into the Circle CI 
5. which slows PR feedback loop as users may not have CircleCI credentials and knowledge of the system

[1]:https://github.com/linkedin/cruise-control/blob/a298df86095532264f13ca7490cfabb8ff68839f/.circleci/config.yml#L51-L53
[2]:https://github.com/linkedin/cruise-control/blob/a298df86095532264f13ca7490cfabb8ff68839f/.circleci/config.yml#L51-L53
[3]:https://github.com/linkedin/cruise-control/blob/a298df86095532264f13ca7490cfabb8ff68839f/.circleci/config.yml#L5-L34
[4]:https://github.com/linkedin/cruise-control/blob/a298df86095532264f13ca7490cfabb8ff68839f/.circleci/config.yml#L94-L103

## Steps to reproduce
1. see failed PR checks, ie #2133

## Known Workarounds
1. asking PR authors to trigger build

## Migration Plan
1. add GH Actions integration along with CircleCI
2. confirm GH Actions provide equivalent or better functionality 
3. remove CircleCI integration
4. ensure publishing via GH actions works

## Categorization
- [x] refactor
Copy link

@adkafka adkafka left a comment

Choose a reason for hiding this comment

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

Nice! Would be great to get a review on this from the maintainers as well, these metrics are valuable to have.

@HerveRiviere
Copy link
Author

We are using in prod these (indeed useful !) metric at Criteo for multiple months now. No specific issue so far.

About the CI , as I'm able to see logs, not sure if is it due to my change or a different issue ?

Thanks !

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.

Add metrics about total and remaining data to rebalance
2 participants