rkanumul commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r621504610
##########
File path:
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -103,9 +107,13 @@
public class HelixServerStarter implements ServiceStartable {
private static final Logger LOGGER =
LoggerFactory.getLogger(HelixServerStarter.class);
+ private static final String ENVIRONMENT_IDENTIFIER = "environment";
+ private static final String FAILURE_DOMAIN_IDENTIFIER = "failureDomain";
+ private static final String TRUE = "true";
Review comment:
Boolean.TRUE.toString()
##########
File path:
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -321,6 +357,16 @@ public void start()
LOGGER.info("Initializing server instance and registering state model
factory");
Utils.logVersions();
ControllerLeaderLocator.create(_helixManager);
+
+ // Invoke pinot environment provider factory's init method to register the
environment provider
+ // and fetch the overridden server configs for the invoked environment
provider
+ if (!skipPinotEnvironmentProviderFactoryInit) {
+
PinotEnvironmentProviderFactory.init(_serverConf.subset(PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY));
+ PinotEnvironmentProvider pinotEnvironmentProvider =
PinotEnvironmentProviderFactory.getEnvironmentProvider(
+ _serverConf.getProperty(CONFIG_OF_ENVIRONMENT_PROVIDER_TYPE));
+ _overriddenServerConfigs =
pinotEnvironmentProvider.getEnvironment(_serverConf.toMap());
Review comment:
why have overridden configs different from server config ?
it is "overidden" with the values appropriate to environment.
this is the new server config now.
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -321,6 +321,11 @@
public static final String CONFIG_OF_ENABLE_THREAD_CPU_TIME_MEASUREMENT =
"pinot.server.instance.enableThreadCpuTimeMeasurement";
public static final boolean DEFAULT_ENABLE_THREAD_CPU_TIME_MEASUREMENT =
false;
+
+ // Environment Provider Configs
+ public static final String
PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY =
"pinot.server.environmentProvider.factory";
+ public static final String CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY =
"pinot.server.environmentProvider.enabled";
+ public static final String CONFIG_OF_ENVIRONMENT_PROVIDER_TYPE =
"pinot.server.environmentProvider.type";
Review comment:
what is type ?
##########
File path:
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -117,13 +125,18 @@
private AdminApiApplication _adminApiApplication;
private ServerQueriesDisabledTracker _serverQueriesDisabledTracker;
private RealtimeLuceneIndexRefreshState _realtimeLuceneIndexRefreshState;
+ private boolean skipPinotEnvironmentProviderFactoryInit = true;
- public HelixServerStarter(String helixClusterName, String zkAddress,
PinotConfiguration serverConf)
- throws Exception {
+ public HelixServerStarter(String helixClusterName, String zkAddress,
PinotConfiguration serverConf) throws Exception {
_helixClusterName = helixClusterName;
_zkAddress = zkAddress;
// Make a clone so that changes to the config won't propagate to the caller
_serverConf = serverConf.clone();
+ // Check if PinotEnvironmentProviderFactoryInit is enabled or not
+ if (_serverConf.containsKey(CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY)
&&
+
TRUE.equals(serverConf.getProperty(CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY)))
{
+ skipPinotEnvironmentProviderFactoryInit = false;
+ }
Review comment:
this check can go directly to wherever you are using the variable. no
need to have this additional variable.
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/utils/CommonConstants.java
##########
@@ -321,6 +321,11 @@
public static final String CONFIG_OF_ENABLE_THREAD_CPU_TIME_MEASUREMENT =
"pinot.server.instance.enableThreadCpuTimeMeasurement";
public static final boolean DEFAULT_ENABLE_THREAD_CPU_TIME_MEASUREMENT =
false;
+
+ // Environment Provider Configs
+ public static final String
PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY =
"pinot.server.environmentProvider.factory";
+ public static final String CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY =
"pinot.server.environmentProvider.enabled";
Review comment:
do we need this ? is not having an env provider same as "false" ?
##########
File path:
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -103,9 +107,13 @@
public class HelixServerStarter implements ServiceStartable {
private static final Logger LOGGER =
LoggerFactory.getLogger(HelixServerStarter.class);
+ private static final String ENVIRONMENT_IDENTIFIER = "environment";
+ private static final String FAILURE_DOMAIN_IDENTIFIER = "failureDomain";
Review comment:
these can be in common constants ? given this config can be populated by
other components as well ?
##########
File path:
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -117,13 +125,18 @@
private AdminApiApplication _adminApiApplication;
private ServerQueriesDisabledTracker _serverQueriesDisabledTracker;
private RealtimeLuceneIndexRefreshState _realtimeLuceneIndexRefreshState;
+ private boolean skipPinotEnvironmentProviderFactoryInit = true;
- public HelixServerStarter(String helixClusterName, String zkAddress,
PinotConfiguration serverConf)
- throws Exception {
+ public HelixServerStarter(String helixClusterName, String zkAddress,
PinotConfiguration serverConf) throws Exception {
_helixClusterName = helixClusterName;
_zkAddress = zkAddress;
// Make a clone so that changes to the config won't propagate to the caller
_serverConf = serverConf.clone();
+ // Check if PinotEnvironmentProviderFactoryInit is enabled or not
+ if (_serverConf.containsKey(CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY)
&&
+
TRUE.equals(serverConf.getProperty(CONFIG_OF_ENABLE_ENVIRONMENT_PROVIDER_FACTORY)))
{
+ skipPinotEnvironmentProviderFactoryInit = false;
+ }
Review comment:
can we not get the overridden configs here before anything happens at
all ?
that way we can make the overidden configs as the new set of configs for the
server
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
