HolyLow commented on code in PR #3013: URL: https://github.com/apache/celeborn/pull/3013#discussion_r1895205858
########## cpp/celeborn/conf/BaseConf.h: ########## @@ -0,0 +1,263 @@ +/* + * Based on Config.h from Facebook Velox + * + * Copyright (c) Facebook, Inc. and its affiliates. + * + * Licensed 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. + */ + +#pragma once + +#include <folly/Conv.h> +#include <folly/Optional.h> +#include <folly/SocketAddress.h> +#include <folly/Synchronized.h> +#include <set> +#include <string> +#include <typeindex> +#include <unordered_map> + +#include "celeborn/utils/Exceptions.h" + +namespace celeborn { + +class Config { + public: + virtual ~Config() = default; + + virtual folly::Optional<std::string> get(const std::string& key) const = 0; + // virtual const string operator[](const std::string& key) = 0; + // overload and disable not supported cases. + + template <typename T> + folly::Optional<T> get(const std::string& key) const { + auto val = get(key); + if (val.hasValue()) { + return folly::to<T>(val.value()); + } else { + return folly::none; + } + } + + template <typename T> + T get(const std::string& key, const T& defaultValue) const { + auto val = get(key); + if (val.hasValue()) { + return folly::to<T>(val.value()); + } else { + return defaultValue; + } + } + + virtual bool isValueExists(const std::string& key) const = 0; + + virtual const std::unordered_map<std::string, std::string>& values() const { + CELEBORN_UNSUPPORTED("method values() is not supported by this config"); + } + + virtual std::unordered_map<std::string, std::string> valuesCopy() const { + CELEBORN_UNSUPPORTED("method valuesCopy() is not supported by this config"); + } +}; + +namespace core { + +class MemConfig : public Config { + public: + explicit MemConfig(const std::unordered_map<std::string, std::string>& values) + : values_(values) {} + + explicit MemConfig() : values_{} {} + + explicit MemConfig(std::unordered_map<std::string, std::string>&& values) + : values_(std::move(values)) {} + + folly::Optional<std::string> get(const std::string& key) const override; + + bool isValueExists(const std::string& key) const override; + + const std::unordered_map<std::string, std::string>& values() const override { + return values_; + } + + std::unordered_map<std::string, std::string> valuesCopy() const override { + return values_; + } + + private: + std::unordered_map<std::string, std::string> values_; +}; + +/// In-memory config allowing changing properties at runtime. +class MemConfigMutable : public Config { + public: + explicit MemConfigMutable( + const std::unordered_map<std::string, std::string>& values) + : values_(values) {} + + explicit MemConfigMutable() : values_{} {} + + explicit MemConfigMutable( + std::unordered_map<std::string, std::string>&& values) + : values_(std::move(values)) {} + + folly::Optional<std::string> get(const std::string& key) const override; + + bool isValueExists(const std::string& key) const override; + + const std::unordered_map<std::string, std::string>& values() const override { + CELEBORN_UNSUPPORTED( + "Mutable config cannot return unprotected reference to values."); + return *values_.rlock(); + } + + std::unordered_map<std::string, std::string> valuesCopy() const override { + return *values_.rlock(); + } + + /// Adds or replaces value at the given key. Can be used by debugging or + /// testing code. + void setValue(const std::string& key, const std::string& value) { + (*values_.wlock())[key] = value; + } + + private: + folly::Synchronized<std::unordered_map<std::string, std::string>> values_; +}; + +} // namespace core + +class BaseConf { + public: + // Setting this to 'true' makes configs modifiable via server operations. + static constexpr std::string_view kMutableConfig{"mutable-config"}; Review Comment: For a general-purpose config system, immutable configs are required because many system-wide configs are not modifiable, such as service thread num, execution mode, etc. For these configs, immutability is by design, and modifying them at runtime would not take effect. The Mutability design here is to enforce the configs to be not modifiable. If this is not aligned to Java end's behavior, we could modify the default mode to mutable. Let me know if the modification is required, thanks a lot. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
