-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Change SharedArbitrator extra config string based #10685
Conversation
✅ Deploy Preview for meta-velox canceled.
|
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
a16ec0a
to
62cd2dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tanjialiang thanks for the refactor % nits.
velox/common/base/ConfigUtil.h
Outdated
PETABYTE | ||
}; | ||
|
||
double toBytesPerCapacityUnit(CapacityUnit unit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add unit tests for these? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I can
velox/common/base/ConfigUtil.cpp
Outdated
|
||
#include "velox/common/base/ConfigUtil.h" | ||
|
||
namespace facebook::velox::config { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume there is no functional change? Why not put this into velox/common/config/config.h?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a completely different ConfigBase class that is only used by dwrf and has nothing to do with this. But it will be nice to do some followup cleanup for our configs.
extraArbitratorConfigs["memory-pool-reserved-capacity"] = | ||
folly::to<std::string>(options.memoryPoolReservedCapacity); | ||
folly::to<std::string>(options.memoryPoolReservedCapacity) + "B"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use utility for this conversion instead of attaching a unit string which seems to be a bit hack. Do we already have such utilities in Prestissimo or Velox? thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is temporary. It will be removed very soon
62cd2dc
to
1f5cec5
Compare
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tanjialiang LGTM. Thanks!
1f5cec5
to
87e653f
Compare
@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…10685) Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang
This pull request was exported from Phabricator. Differential Revision: D60920773 |
87e653f
to
5406019
Compare
@tanjialiang merged this pull request in fe9605f. |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…10685) Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
…10685) Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Summary: Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions. Pull Request resolved: facebookincubator#10685 Reviewed By: xiaoxmeng Differential Revision: D60920773 Pulled By: tanjialiang fbshipit-source-id: 215d4e45d89c0b319f883a7c4b1ed27abd1c3e9f
Switch to string based config for extra configs in shared arbitrator for better consistency and fewer back and forth conversions.