qqu0127 commented on code in PR #2249: URL: https://github.com/apache/helix/pull/2249#discussion_r1001191813
########## meta-client/src/main/java/org/apache/helix/metaclient/factories/MetaClientFactory.java: ########## @@ -0,0 +1,17 @@ +package org.apache.helix.metaclient.factories; + Review Comment: Add license ########## meta-client/src/main/java/org/apache/helix/metaclient/api/ConnectStateChangeListener.java: ########## @@ -0,0 +1,36 @@ +package org.apache.helix.metaclient.api; + +/* + * 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. + */ + +public interface ConnectStateChangeListener { + /** + * Called when the connection state has changed. + */ + void handleConnectStateChanged(MetaClientInterface.ConnectState currentState) throws Exception; + /** + * Called when new connection is established + */ + void handleNewConnection(final String sessionId) throws Exception; + /** + * Called when new connection failed to established + */ + void handleConnectionEstablishmentError(final Throwable error) throws Exception; Review Comment: Do we know what type of exception might be thrown? I feel it doesn't add much value with unchecked exception in the interface. ########## meta-client/src/main/java/org/apache/helix/metaclient/factories/MetaClientConfig.java: ########## @@ -0,0 +1,91 @@ +package org.apache.helix.metaclient.factories; + +class MetaClientConfig { + + public enum RetryProtocol { + NO_RETRY, EXP_BACK_OFF, CONST_RETRY_INTERVAL + } Review Comment: RetryProtocol should better be a standalone class, we may want to add their custom logic and make it self-contained. ########## meta-client/src/main/java/org/apache/helix/metaclient/factories/MetaClientConfig.java: ########## @@ -0,0 +1,91 @@ +package org.apache.helix.metaclient.factories; + Review Comment: Add license ########## meta-client/src/main/java/org/apache/helix/metaclient/factories/MetaClientConfig.java: ########## @@ -0,0 +1,91 @@ +package org.apache.helix.metaclient.factories; + +class MetaClientConfig { + + public enum RetryProtocol { + NO_RETRY, EXP_BACK_OFF, CONST_RETRY_INTERVAL + } + + public enum StoreType { + ZOOKEEPER, ETCD + } + + private String _connectionAddress; + private long _connectionTimeout; + private boolean _enableAuth; + private StoreType _storeType; Review Comment: with builder pattern, all these can be defined as final ########## meta-client/src/main/java/org/apache/helix/metaclient/impl/zk/ZkMetaClient.java: ########## @@ -0,0 +1,211 @@ +package org.apache.helix.metaclient.impl.zk; + Review Comment: Add license ########## meta-client/src/main/java/org/apache/helix/metaclient/factories/MetaClientConfig.java: ########## @@ -0,0 +1,91 @@ +package org.apache.helix.metaclient.factories; + +class MetaClientConfig { + + public enum RetryProtocol { + NO_RETRY, EXP_BACK_OFF, CONST_RETRY_INTERVAL + } + + public enum StoreType { + ZOOKEEPER, ETCD + } + + private String _connectionAddress; + private long _connectionTimeout; + private boolean _enableAuth; + private StoreType _storeType; + + public String getConnectionAddress() { + return _connectionAddress; + } + + public long getConnectionTimeout() { + return _connectionTimeout; + } + + public boolean isAuthEnabled() { + return _enableAuth; + } + public StoreType getStoreType() { + return _storeType; + } + + // TODO: More options to add later + // private boolean _autoReRegistWatcher; // re-register one time watcher when set to true + // private boolean _resetWatchWhenReConnect; // re-register previous existing watcher when reconnect + // private RetryProtocol _retryProtocol; + + private MetaClientConfig(Builder builder) { + _connectionAddress = builder._connectionAddress; + _connectionTimeout = builder._connectionTimeout; + _enableAuth = builder._enableAuth; + _storeType = builder._storeType; + + } Review Comment: Let's input every single variables, but not depends on builder ########## meta-client/src/main/java/org/apache/helix/metaclient/api/DirectEntrySubscribeResult.java: ########## @@ -0,0 +1,14 @@ +package org.apache.helix.metaclient.api; + Review Comment: Add license ########## meta-client/src/main/java/org/apache/helix/metaclient/factories/MetaClientConfig.java: ########## @@ -0,0 +1,91 @@ +package org.apache.helix.metaclient.factories; + +class MetaClientConfig { + + public enum RetryProtocol { + NO_RETRY, EXP_BACK_OFF, CONST_RETRY_INTERVAL + } + + public enum StoreType { + ZOOKEEPER, ETCD + } + + private String _connectionAddress; + private long _connectionTimeout; + private boolean _enableAuth; + private StoreType _storeType; + + public String getConnectionAddress() { + return _connectionAddress; + } + + public long getConnectionTimeout() { + return _connectionTimeout; + } + + public boolean isAuthEnabled() { + return _enableAuth; + } + public StoreType getStoreType() { + return _storeType; + } + + // TODO: More options to add later + // private boolean _autoReRegistWatcher; // re-register one time watcher when set to true + // private boolean _resetWatchWhenReConnect; // re-register previous existing watcher when reconnect + // private RetryProtocol _retryProtocol; + + private MetaClientConfig(Builder builder) { + _connectionAddress = builder._connectionAddress; + _connectionTimeout = builder._connectionTimeout; + _enableAuth = builder._enableAuth; + _storeType = builder._storeType; + + } + + public static class Builder { + private String _connectionAddress; + + private long _connectionTimeout; + private boolean _enableAuth; + //private RetryProtocol _retryProtocol; + private StoreType _storeType; + + + + public MetaClientConfig build() { + validate(); + return new MetaClientConfig(this); Review Comment: Exposing "this" isn't a good practice, let's try to avoid -- 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]
