You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "HolyLow (via GitHub)" <gi...@apache.org> on 2024/12/24 09:13:15 UTC
[PR] [CELEBORN-1799][CIP-14] Add celebornConf to cppClient [celeborn]
HolyLow opened a new pull request, #3027:
URL: https://github.com/apache/celeborn/pull/3027
### What changes were proposed in this pull request?
Add CelebornConf to cppClient.
### Why are the changes needed?
The CelebornConf will be used as configuration module in cppClient.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Compilation and UTs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1799][CIP-14] Add celebornConf to cppClient [celeborn]
Posted by "HolyLow (via GitHub)" <gi...@apache.org>.
HolyLow commented on code in PR #3027:
URL: https://github.com/apache/celeborn/pull/3027#discussion_r1897163511
##########
cpp/celeborn/conf/CelebornConf.cpp:
##########
@@ -0,0 +1,198 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <re2/re2.h>
+
+#include "celeborn/conf/CelebornConf.h"
+
+namespace celeborn {
+namespace {
+
+// folly::to<> does not generate 'true' and 'false', so we do it ourselves.
+std::string bool2String(bool value) {
+ return value ? "true" : "false";
+}
+
+#define STR_PROP(_key_, _val_) {std::string(_key_), std::string(_val_)}
+#define NUM_PROP(_key_, _val_) \
+ {std::string(_key_), folly::to<std::string>(_val_)}
+#define BOOL_PROP(_key_, _val_) {std::string(_key_), bool2String(_val_)}
+#define NONE_PROP(_key_) {std::string(_key_), folly::none}
+
+enum class CapacityUnit {
+ BYTE,
+ KILOBYTE,
+ MEGABYTE,
+ GIGABYTE,
+ TERABYTE,
+ PETABYTE
+};
+
+double toBytesPerCapacityUnit(CapacityUnit unit) {
+ switch (unit) {
+ case CapacityUnit::BYTE:
+ return 1;
+ case CapacityUnit::KILOBYTE:
+ return exp2(10);
+ case CapacityUnit::MEGABYTE:
+ return exp2(20);
+ case CapacityUnit::GIGABYTE:
+ return exp2(30);
+ case CapacityUnit::TERABYTE:
+ return exp2(40);
+ case CapacityUnit::PETABYTE:
+ return exp2(50);
+ default:
+ CELEBORN_USER_FAIL("Invalid capacity unit '{}'", (int)unit);
+ }
+}
+
+CapacityUnit valueOfCapacityUnit(const std::string& unitStr) {
+ if (unitStr == "B") {
+ return CapacityUnit::BYTE;
+ }
+ if (unitStr == "kB") {
+ return CapacityUnit::KILOBYTE;
+ }
+ if (unitStr == "MB") {
+ return CapacityUnit::MEGABYTE;
+ }
+ if (unitStr == "GB") {
+ return CapacityUnit::GIGABYTE;
+ }
+ if (unitStr == "TB") {
+ return CapacityUnit::TERABYTE;
+ }
+ if (unitStr == "PB") {
+ return CapacityUnit::PETABYTE;
+ }
+ CELEBORN_USER_FAIL("Invalid capacity unit '{}'", unitStr);
+}
+
+// Convert capacity string with unit to the capacity number in the specified
+// units
+uint64_t toCapacity(const std::string& from, CapacityUnit to) {
+ static const RE2 kPattern(R"(^\s*(\d+(?:\.\d+)?)\s*([a-zA-Z]+)\s*$)");
+ double value;
+ std::string unit;
+ if (!RE2::FullMatch(from, kPattern, &value, &unit)) {
+ CELEBORN_USER_FAIL("Invalid capacity string '{}'", from);
+ }
+
+ return value *
+ (toBytesPerCapacityUnit(valueOfCapacityUnit(unit)) /
+ toBytesPerCapacityUnit(to));
+}
+
+Duration toDuration(const std::string& str) {
+ static const RE2 kPattern(R"(^\s*(\d+(?:\.\d+)?)\s*([a-zA-Z]+)\s*)");
+
+ double value;
+ std::string unit;
+ if (!RE2::FullMatch(str, kPattern, &value, &unit)) {
+ CELEBORN_USER_FAIL("Invalid duration {}", str);
+ }
+ if (unit == "ns") {
+ return std::chrono::duration<double, std::nano>(value);
+ } else if (unit == "us") {
+ return std::chrono::duration<double, std::micro>(value);
+ } else if (unit == "ms") {
+ return std::chrono::duration<double, std::milli>(value);
+ } else if (unit == "s") {
+ return Duration(value);
+ } else if (unit == "m") {
+ return std::chrono::duration<double, std::ratio<60>>(value);
+ } else if (unit == "h") {
+ return std::chrono::duration<double, std::ratio<60 * 60>>(value);
+ } else if (unit == "d") {
+ return std::chrono::duration<double, std::ratio<60 * 60 * 24>>(value);
+ }
+ CELEBORN_USER_FAIL("Invalid duration {}", str);
+}
+
+} // namespace
+
+const std::unordered_map<std::string, folly::Optional<std::string>>
+ CelebornConf::kDefaultProperties = {
+ STR_PROP(kRpcLookupTimeout, "30s"),
+ STR_PROP(kClientRpcGetReducerFileGroupRpcAskTimeout, "240s"),
Review Comment:
Modified as required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1799][CIP-14] Add celebornConf to cppClient [celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3027:
URL: https://github.com/apache/celeborn/pull/3027#issuecomment-2560893507
## [Codecov](https://app.codecov.io/gh/apache/celeborn/pull/3027?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 32.68%. Comparing base [(`2eb4c23`)](https://app.codecov.io/gh/apache/celeborn/commit/2eb4c23eb8c87788daea66caa8e331e024d160b5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`f9e3602`)](https://app.codecov.io/gh/apache/celeborn/commit/f9e3602eb7b18547e861faf00c427217435e19eb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 4 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #3027 +/- ##
==========================================
- Coverage 32.99% 32.68% -0.30%
==========================================
Files 331 336 +5
Lines 19842 20032 +190
Branches 1787 1792 +5
==========================================
+ Hits 6545 6546 +1
- Misses 12933 13123 +190
+ Partials 364 363 -1
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/celeborn/pull/3027?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1799][CIP-14] Add celebornConf to cppClient [celeborn]
Posted by "HolyLow (via GitHub)" <gi...@apache.org>.
HolyLow commented on PR #3027:
URL: https://github.com/apache/celeborn/pull/3027#issuecomment-2560878948
@FMX Could you kindly review this PR? Thanks a lot.
Any suggestion is welcome.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1799][CIP-14] Add celebornConf to cppClient [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #3027:
URL: https://github.com/apache/celeborn/pull/3027#discussion_r1897066523
##########
cpp/celeborn/conf/CelebornConf.cpp:
##########
@@ -0,0 +1,198 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <re2/re2.h>
+
+#include "celeborn/conf/CelebornConf.h"
+
+namespace celeborn {
+namespace {
+
+// folly::to<> does not generate 'true' and 'false', so we do it ourselves.
+std::string bool2String(bool value) {
+ return value ? "true" : "false";
+}
+
+#define STR_PROP(_key_, _val_) {std::string(_key_), std::string(_val_)}
+#define NUM_PROP(_key_, _val_) \
+ {std::string(_key_), folly::to<std::string>(_val_)}
+#define BOOL_PROP(_key_, _val_) {std::string(_key_), bool2String(_val_)}
+#define NONE_PROP(_key_) {std::string(_key_), folly::none}
+
+enum class CapacityUnit {
+ BYTE,
+ KILOBYTE,
+ MEGABYTE,
+ GIGABYTE,
+ TERABYTE,
+ PETABYTE
+};
+
+double toBytesPerCapacityUnit(CapacityUnit unit) {
+ switch (unit) {
+ case CapacityUnit::BYTE:
+ return 1;
+ case CapacityUnit::KILOBYTE:
+ return exp2(10);
+ case CapacityUnit::MEGABYTE:
+ return exp2(20);
+ case CapacityUnit::GIGABYTE:
+ return exp2(30);
+ case CapacityUnit::TERABYTE:
+ return exp2(40);
+ case CapacityUnit::PETABYTE:
+ return exp2(50);
+ default:
+ CELEBORN_USER_FAIL("Invalid capacity unit '{}'", (int)unit);
+ }
+}
+
+CapacityUnit valueOfCapacityUnit(const std::string& unitStr) {
+ if (unitStr == "B") {
+ return CapacityUnit::BYTE;
+ }
+ if (unitStr == "kB") {
+ return CapacityUnit::KILOBYTE;
+ }
+ if (unitStr == "MB") {
+ return CapacityUnit::MEGABYTE;
+ }
+ if (unitStr == "GB") {
+ return CapacityUnit::GIGABYTE;
+ }
+ if (unitStr == "TB") {
+ return CapacityUnit::TERABYTE;
+ }
+ if (unitStr == "PB") {
+ return CapacityUnit::PETABYTE;
+ }
+ CELEBORN_USER_FAIL("Invalid capacity unit '{}'", unitStr);
+}
+
+// Convert capacity string with unit to the capacity number in the specified
+// units
+uint64_t toCapacity(const std::string& from, CapacityUnit to) {
+ static const RE2 kPattern(R"(^\s*(\d+(?:\.\d+)?)\s*([a-zA-Z]+)\s*$)");
+ double value;
+ std::string unit;
+ if (!RE2::FullMatch(from, kPattern, &value, &unit)) {
+ CELEBORN_USER_FAIL("Invalid capacity string '{}'", from);
+ }
+
+ return value *
+ (toBytesPerCapacityUnit(valueOfCapacityUnit(unit)) /
+ toBytesPerCapacityUnit(to));
+}
+
+Duration toDuration(const std::string& str) {
+ static const RE2 kPattern(R"(^\s*(\d+(?:\.\d+)?)\s*([a-zA-Z]+)\s*)");
+
+ double value;
+ std::string unit;
+ if (!RE2::FullMatch(str, kPattern, &value, &unit)) {
+ CELEBORN_USER_FAIL("Invalid duration {}", str);
+ }
+ if (unit == "ns") {
+ return std::chrono::duration<double, std::nano>(value);
+ } else if (unit == "us") {
+ return std::chrono::duration<double, std::micro>(value);
+ } else if (unit == "ms") {
+ return std::chrono::duration<double, std::milli>(value);
+ } else if (unit == "s") {
+ return Duration(value);
+ } else if (unit == "m") {
+ return std::chrono::duration<double, std::ratio<60>>(value);
+ } else if (unit == "h") {
+ return std::chrono::duration<double, std::ratio<60 * 60>>(value);
+ } else if (unit == "d") {
+ return std::chrono::duration<double, std::ratio<60 * 60 * 24>>(value);
+ }
+ CELEBORN_USER_FAIL("Invalid duration {}", str);
+}
+
+} // namespace
+
+const std::unordered_map<std::string, folly::Optional<std::string>>
+ CelebornConf::kDefaultProperties = {
+ STR_PROP(kRpcLookupTimeout, "30s"),
+ STR_PROP(kClientRpcGetReducerFileGroupRpcAskTimeout, "240s"),
Review Comment:
The default value of this config is 30s. We should align the default values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1799][CIP-14] Add celebornConf to cppClient [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX closed pull request #3027: [CELEBORN-1799][CIP-14] Add celebornConf to cppClient
URL: https://github.com/apache/celeborn/pull/3027
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1799][CIP-14] Add celebornConf to cppClient [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #3027:
URL: https://github.com/apache/celeborn/pull/3027#discussion_r1897066523
##########
cpp/celeborn/conf/CelebornConf.cpp:
##########
@@ -0,0 +1,198 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <re2/re2.h>
+
+#include "celeborn/conf/CelebornConf.h"
+
+namespace celeborn {
+namespace {
+
+// folly::to<> does not generate 'true' and 'false', so we do it ourselves.
+std::string bool2String(bool value) {
+ return value ? "true" : "false";
+}
+
+#define STR_PROP(_key_, _val_) {std::string(_key_), std::string(_val_)}
+#define NUM_PROP(_key_, _val_) \
+ {std::string(_key_), folly::to<std::string>(_val_)}
+#define BOOL_PROP(_key_, _val_) {std::string(_key_), bool2String(_val_)}
+#define NONE_PROP(_key_) {std::string(_key_), folly::none}
+
+enum class CapacityUnit {
+ BYTE,
+ KILOBYTE,
+ MEGABYTE,
+ GIGABYTE,
+ TERABYTE,
+ PETABYTE
+};
+
+double toBytesPerCapacityUnit(CapacityUnit unit) {
+ switch (unit) {
+ case CapacityUnit::BYTE:
+ return 1;
+ case CapacityUnit::KILOBYTE:
+ return exp2(10);
+ case CapacityUnit::MEGABYTE:
+ return exp2(20);
+ case CapacityUnit::GIGABYTE:
+ return exp2(30);
+ case CapacityUnit::TERABYTE:
+ return exp2(40);
+ case CapacityUnit::PETABYTE:
+ return exp2(50);
+ default:
+ CELEBORN_USER_FAIL("Invalid capacity unit '{}'", (int)unit);
+ }
+}
+
+CapacityUnit valueOfCapacityUnit(const std::string& unitStr) {
+ if (unitStr == "B") {
+ return CapacityUnit::BYTE;
+ }
+ if (unitStr == "kB") {
+ return CapacityUnit::KILOBYTE;
+ }
+ if (unitStr == "MB") {
+ return CapacityUnit::MEGABYTE;
+ }
+ if (unitStr == "GB") {
+ return CapacityUnit::GIGABYTE;
+ }
+ if (unitStr == "TB") {
+ return CapacityUnit::TERABYTE;
+ }
+ if (unitStr == "PB") {
+ return CapacityUnit::PETABYTE;
+ }
+ CELEBORN_USER_FAIL("Invalid capacity unit '{}'", unitStr);
+}
+
+// Convert capacity string with unit to the capacity number in the specified
+// units
+uint64_t toCapacity(const std::string& from, CapacityUnit to) {
+ static const RE2 kPattern(R"(^\s*(\d+(?:\.\d+)?)\s*([a-zA-Z]+)\s*$)");
+ double value;
+ std::string unit;
+ if (!RE2::FullMatch(from, kPattern, &value, &unit)) {
+ CELEBORN_USER_FAIL("Invalid capacity string '{}'", from);
+ }
+
+ return value *
+ (toBytesPerCapacityUnit(valueOfCapacityUnit(unit)) /
+ toBytesPerCapacityUnit(to));
+}
+
+Duration toDuration(const std::string& str) {
+ static const RE2 kPattern(R"(^\s*(\d+(?:\.\d+)?)\s*([a-zA-Z]+)\s*)");
+
+ double value;
+ std::string unit;
+ if (!RE2::FullMatch(str, kPattern, &value, &unit)) {
+ CELEBORN_USER_FAIL("Invalid duration {}", str);
+ }
+ if (unit == "ns") {
+ return std::chrono::duration<double, std::nano>(value);
+ } else if (unit == "us") {
+ return std::chrono::duration<double, std::micro>(value);
+ } else if (unit == "ms") {
+ return std::chrono::duration<double, std::milli>(value);
+ } else if (unit == "s") {
+ return Duration(value);
+ } else if (unit == "m") {
+ return std::chrono::duration<double, std::ratio<60>>(value);
+ } else if (unit == "h") {
+ return std::chrono::duration<double, std::ratio<60 * 60>>(value);
+ } else if (unit == "d") {
+ return std::chrono::duration<double, std::ratio<60 * 60 * 24>>(value);
+ }
+ CELEBORN_USER_FAIL("Invalid duration {}", str);
+}
+
+} // namespace
+
+const std::unordered_map<std::string, folly::Optional<std::string>>
+ CelebornConf::kDefaultProperties = {
+ STR_PROP(kRpcLookupTimeout, "30s"),
+ STR_PROP(kClientRpcGetReducerFileGroupRpcAskTimeout, "240s"),
Review Comment:
The default value of this config is 60s. We should align the default values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org