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

feat: Add Global Config in place of gflags #11745

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

Conversation

majetideepak
Copy link
Collaborator

@majetideepak majetideepak commented Dec 4, 2024

Remove usage of gflags from velox_memory and velox_exception

@majetideepak majetideepak requested a review from kgpai December 4, 2024 20:16
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 4, 2024
Copy link

netlify bot commented Dec 4, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 4bc9d63
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/6756f7ba0cb6f4000815b367

velox/common/config/Config.h Outdated Show resolved Hide resolved
@majetideepak majetideepak force-pushed the remove-gflags branch 2 times, most recently from 7f84965 to 47a3a23 Compare December 4, 2024 23:49
@majetideepak
Copy link
Collaborator Author

@Yuhta can you take another look? I modified it according to your suggestion.

@majetideepak majetideepak force-pushed the remove-gflags branch 2 times, most recently from 046911b to 888dd49 Compare December 5, 2024 18:39
@Yuhta
Copy link
Contributor

Yuhta commented Dec 5, 2024

@majetideepak Yes it looks good now. We also need a translation function from gflags to this new settings, so that people can do the migration seamlessly.

@majetideepak
Copy link
Collaborator Author

@Yuhta can you give an example for this translation function? My idea is that we can add these flags in Prestissimo (or other velox application) and translate to this global config. libvelox will not have any dependency on gflags.
More details here #11730

@majetideepak
Copy link
Collaborator Author

I guess we can create the translation function in a separate library and map the gflag to this global config. I will add this function.

@majetideepak majetideepak force-pushed the remove-gflags branch 2 times, most recently from d961d89 to 7ef8932 Compare December 6, 2024 19:31
@majetideepak
Copy link
Collaborator Author

@Yuhta Can you take another look? Thanks.

velox/common/config/GlobalConfig.h Outdated Show resolved Hide resolved
velox/buffer/tests/BufferTest.cpp Outdated Show resolved Hide resolved
@majetideepak majetideepak changed the title feat: Add Global Config feat: Add Global Config in place of gflags Dec 6, 2024
@majetideepak majetideepak force-pushed the remove-gflags branch 3 times, most recently from a4bdf43 to 41690d4 Compare December 7, 2024 11:11
@majetideepak majetideepak marked this pull request as ready for review December 7, 2024 20:27
@majetideepak
Copy link
Collaborator Author

@Yuhta This is ready for another pass.

@facebook-github-bot
Copy link
Contributor

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants