rahulrane50 commented on code in PR #2291:
URL: https://github.com/apache/helix/pull/2291#discussion_r1054995002
##########
meta-client/src/main/java/org/apache/helix/metaclient/factories/MetaClientConfig.java:
##########
@@ -19,23 +19,32 @@
* under the License.
*/
-class MetaClientConfig {
+import org.apache.helix.metaclient.constants.MetaClientConstants;
+
+public class MetaClientConfig {
public enum StoreType {
ZOOKEEPER, ETCD, CUSTOMIZED
Review Comment:
Interesting to see "etcd" here. Should we mention this here yet or just say
customized? (could there be licensing issue? )
##########
meta-client/src/main/java/org/apache/helix/metaclient/factories/MetaClientConfig.java:
##########
@@ -56,56 +69,82 @@ public StoreType getStoreType() {
// private RetryProtocol _retryProtocol;
- private MetaClientConfig(String connectionAddress, long connectionTimeout,
boolean enableAuth,
- StoreType storeType) {
+ protected MetaClientConfig(String connectionAddress, long
connectionInitTimeoutInMillis,
+ long sessionTimeoutInMillis, boolean enableAuth, StoreType storeType) {
_connectionAddress = connectionAddress;
- _connectionTimeout = connectionTimeout;
+ _connectionInitTimeoutInMillis = connectionInitTimeoutInMillis;
+ _sessionTimeoutInMillis = sessionTimeoutInMillis;
_enableAuth = enableAuth;
_storeType = storeType;
}
- public static class Builder {
- private String _connectionAddress;
-
- private long _connectionTimeout;
- private boolean _enableAuth;
- //private RetryProtocol _retryProtocol;
- private StoreType _storeType;
+ public static class MetaClientConfigBuilder<B extends
MetaClientConfigBuilder<B>> {
+ protected String _connectionAddress;
+ protected long _connectionInitTimeoutInMillis;
+ protected long _sessionTimeoutInMillis;
+ // protected long _operationRetryTimeout;
+ // protected RetryProtocol _retryProtocol;
+ protected boolean _enableAuth;
+ protected StoreType _storeType;
public MetaClientConfig build() {
validate();
- return new MetaClientConfig(_connectionAddress, _connectionTimeout,
_enableAuth, _storeType);
+ return new MetaClientConfig(_connectionAddress,
_connectionInitTimeoutInMillis,
+ _sessionTimeoutInMillis,
+ _enableAuth, _storeType);
}
- public Builder() {
+ public MetaClientConfigBuilder() {
// set default values
setAuthEnabled(false);
- setConnectionTimeout(-1);
+
setConnectionInitTimeoutInMillis(MetaClientConstants.DEFAULT_CONNECTION_INIT_TIMEOUT_MS);
+
setSessionTimeoutInMillis(MetaClientConstants.DEFAULT_SESSION_TIMEOUT_MS);
}
- public Builder setConnectionAddress(String connectionAddress) {
+ public B setConnectionAddress(String connectionAddress) {
Review Comment:
Curious what is this "B"? I'm assuming it's template class but isn't it
usually referred as "T"? Same question for line 81 as well
##########
meta-client/src/main/java/org/apache/helix/metaclient/factories/MetaClientConfig.java:
##########
@@ -19,23 +19,32 @@
* under the License.
*/
-class MetaClientConfig {
+import org.apache.helix.metaclient.constants.MetaClientConstants;
+
+public class MetaClientConfig {
public enum StoreType {
ZOOKEEPER, ETCD, CUSTOMIZED
}
private final String _connectionAddress;
- private final long _connectionTimeout;
+
Review Comment:
Neat: would it be a good idea to add all "connection/session" related
configs in separate class called ConnectionConfig, that way we would just need
to pass ConnectionConfig in builder class and in future we can add more params
related to connection/session without changing much code.
##########
meta-client/src/main/java/org/apache/helix/metaclient/factories/MetaClientConfig.java:
##########
@@ -19,23 +19,32 @@
* under the License.
*/
-class MetaClientConfig {
+import org.apache.helix.metaclient.constants.MetaClientConstants;
+
+public class MetaClientConfig {
public enum StoreType {
ZOOKEEPER, ETCD, CUSTOMIZED
}
private final String _connectionAddress;
- private final long _connectionTimeout;
+
+ // Wait for init timeout time until connection is initiated
+ private final long _connectionInitTimeoutInMillis;
+
+ // When a client becomes partitioned from the metadata service for more than
session timeout,
+ // new session will be established when reconnect.
+ private final long _sessionTimeoutInMillis;
+
private final boolean _enableAuth;
Review Comment:
Should we add this now? I think we should add whole security APIs and config
stuff later a single PR because just _enableAuth won't be enough but whole
"AuthConfig" objection should be passed to builder class. This AuthConfig will
have lot of different configs in it.
##########
meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/factory/ZkMetaClientConfig.java:
##########
@@ -0,0 +1,145 @@
+package org.apache.helix.metaclient.impl.zk.factory;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.metaclient.factories.MetaClientConfig;
+import org.apache.helix.zookeeper.zkclient.serialize.BasicZkSerializer;
+import org.apache.helix.zookeeper.zkclient.serialize.PathBasedZkSerializer;
+import org.apache.helix.zookeeper.zkclient.serialize.SerializableSerializer;
+import org.apache.helix.zookeeper.zkclient.serialize.ZkSerializer;
+
+
+public class ZkMetaClientConfig extends MetaClientConfig {
+
+ protected final PathBasedZkSerializer _zkSerializer;
+
+ // Monitoring related fields. MBean names are crated using following
variables in format of
Review Comment:
neat few typos in comments :)
##########
meta-client/src/main/java/org/apache/helix/metaclient/factories/MetaClientConfig.java:
##########
@@ -56,56 +69,82 @@ public StoreType getStoreType() {
// private RetryProtocol _retryProtocol;
- private MetaClientConfig(String connectionAddress, long connectionTimeout,
boolean enableAuth,
- StoreType storeType) {
+ protected MetaClientConfig(String connectionAddress, long
connectionInitTimeoutInMillis,
+ long sessionTimeoutInMillis, boolean enableAuth, StoreType storeType) {
_connectionAddress = connectionAddress;
- _connectionTimeout = connectionTimeout;
+ _connectionInitTimeoutInMillis = connectionInitTimeoutInMillis;
+ _sessionTimeoutInMillis = sessionTimeoutInMillis;
_enableAuth = enableAuth;
_storeType = storeType;
}
- public static class Builder {
- private String _connectionAddress;
-
- private long _connectionTimeout;
- private boolean _enableAuth;
- //private RetryProtocol _retryProtocol;
- private StoreType _storeType;
+ public static class MetaClientConfigBuilder<B extends
MetaClientConfigBuilder<B>> {
+ protected String _connectionAddress;
+ protected long _connectionInitTimeoutInMillis;
+ protected long _sessionTimeoutInMillis;
+ // protected long _operationRetryTimeout;
+ // protected RetryProtocol _retryProtocol;
+ protected boolean _enableAuth;
+ protected StoreType _storeType;
public MetaClientConfig build() {
validate();
- return new MetaClientConfig(_connectionAddress, _connectionTimeout,
_enableAuth, _storeType);
+ return new MetaClientConfig(_connectionAddress,
_connectionInitTimeoutInMillis,
+ _sessionTimeoutInMillis,
+ _enableAuth, _storeType);
}
- public Builder() {
+ public MetaClientConfigBuilder() {
// set default values
setAuthEnabled(false);
- setConnectionTimeout(-1);
+
setConnectionInitTimeoutInMillis(MetaClientConstants.DEFAULT_CONNECTION_INIT_TIMEOUT_MS);
+
setSessionTimeoutInMillis(MetaClientConstants.DEFAULT_SESSION_TIMEOUT_MS);
}
- public Builder setConnectionAddress(String connectionAddress) {
+ public B setConnectionAddress(String connectionAddress) {
_connectionAddress = connectionAddress;
- return this;
+ return self();
}
- public Builder setAuthEnabled(Boolean enableAuth) {
+ public B setAuthEnabled(Boolean enableAuth) {
_enableAuth = enableAuth;
- return this;
+ return self();
+ }
+
+ /**
+ * Set timeout in mm for connection initialization timeout
+ * @param timeout
+ * @return
+ */
+ public B setConnectionInitTimeoutInMillis(long timeout) {
+ _connectionInitTimeoutInMillis = timeout;
+ return self();
}
- public Builder setConnectionTimeout(long timeout) {
- _connectionTimeout = timeout;
- return this;
+ /**
+ * Set timeout in mm for session timeout. When a client becomes
partitioned from the metadata
+ * service for more than session timeout, new session will be established.
+ * @param timeout
+ * @return
+ */
+ public B setSessionTimeoutInMillis(long timeout) {
+ _sessionTimeoutInMillis = timeout;
+ return self();
}
- public Builder setStoreType(StoreType storeType) {
+ public B setStoreType(StoreType storeType) {
_storeType = storeType;
- return this;
+ return self();
+ }
+
+ @SuppressWarnings("unchecked")
+ final B self() {
+ return (B) this;
}
- private void validate() {
+ protected void validate() {
Review Comment:
We should add more validations here right? Are you planning to do it in
future? I think i can think of is having separate "validate" for each major
config classes like "ConnectionConfig", "AuthConfig" etc and make use of them
here.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
