You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/03/17 00:52:41 UTC
[GitHub] [helix] narendly opened a new pull request #899: Builder refactor
narendly opened a new pull request #899: Builder refactor
URL: https://github.com/apache/helix/pull/899
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on issue #899: Use Java Generics and
inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on issue #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#issuecomment-601778823
> Looks good in general.
> The only thing concerns me is the newly modified deprecated constructors. We should either keep the same behavior as the master branch and then mark them as deprecated, or change them to use the new builder logic completely. The current diff on the zooscalability branch is mixed.
>
> Deprecating them would be easier and safer. But we don't need to touch them in this PR.
> So can we have a small follow-up PR to:
>
> 1. revert the newly modified constructors that are not onboarding to the new builder. Since builder should be the new way we create an accessor/verifier that uses a realm-aware ZkClient, right?
> 2. mark the old constructor deprecated.
This has been discussed offline among other PMC and committers, and we have considered this possibility, but we decided that in order to make rollout and adoption smoother, we have opted for this design. This way, users can easily test out the behavior without having to make any code change. Does that make sense to you?
The behavior in fact is exactly same. The difference kicks in only if the user sets the multiZk System config. Also note that the end-user who uses Helix Java APIs do not know what kind of zkclient is returned - it is an internal detail that belongs inside Java API construction logic.
As far as making a new method deprecated, I think there's a tradeoff here. I do not have to deprecate it if you're not comfortable with it, but the method itself is needed in order to promote code reuse. It's just the logic that should be deprecated.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395347358
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/GenericBaseDataAccessorBuilder.java
##########
@@ -0,0 +1,144 @@
+package org.apache.helix.manager.zk;
+
+/*
+ * 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 java.io.IOException;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.helix.HelixException;
+import org.apache.helix.msdcommon.exception.InvalidRoutingDataException;
+import org.apache.helix.zookeeper.api.client.HelixZkClient;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.apache.helix.zookeeper.api.client.ZkClientType;
+import org.apache.helix.zookeeper.impl.client.FederatedZkClient;
+import org.apache.helix.zookeeper.impl.factory.DedicatedZkClientFactory;
+import org.apache.helix.zookeeper.impl.factory.SharedZkClientFactory;
+
+
+/**
+ * GenericBaseDataAccessorBuilder serves as the abstract parent class for Builders used by
+ * BaseDataAccessor APIs that create ZK connections. By having this class, we promote code-reuse.
+ * @param <B>
+ */
+public class GenericBaseDataAccessorBuilder<B extends GenericBaseDataAccessorBuilder<B>> extends GenericZkHelixApiBuilder<B> {
+ /** ZK-based BaseDataAccessor-specific parameter **/
+ private ZkClientType _zkClientType;
+
+ /**
+ * Sets the ZkClientType.
+ * If this is set to either DEDICATED or SHARED, this accessor will be created on
+ * single-realm mode.
+ * If this is set to FEDERATED, multi-realm mode will be used.
+ * @param zkClientType
+ * @return
+ */
+ public B setZkClientType(ZkBaseDataAccessor.ZkClientType zkClientType) {
+ _zkClientType = Enum.valueOf(ZkClientType.class, zkClientType.name());
+ return self();
Review comment:
nit, return setZkClientType(Enum.valueOf(ZkClientType.class, zkClientType.name()));
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r394670822
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -142,22 +142,22 @@ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
}
private ZkBaseDataAccessor(Builder<T> builder) {
- switch (builder.realmMode) {
+ switch (builder.getRealmMode()) {
case MULTI_REALM:
try {
- if (builder.zkClientType == ZkClientType.DEDICATED) {
+ if (builder._zkClientType == ZkClientType.DEDICATED) {
Review comment:
I would prefer 2. Once a class is designed with a builder, the builder should contain most of the complicated building logic. IMHO, including which client to use. Then the real private constructor of the Accessor just takes whatever client it is given. Although both ways avoid duplicate code, method 2 encapsulates the logic more closely.
Actually, it is an option that you put this logic into the generic builder. For the other accessors, this type can be fixed in the implemented builder class.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395809363
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -384,14 +373,26 @@ protected void validate() {
+ _zkAddress + " RealmAwareZkConnectionConfig: " + _realmAwareZkConnectionConfig);
}
}
+ initializeConfigsIfNull();
+ }
- // Resolve all default values
- if (_realmAwareZkConnectionConfig == null) {
- _realmAwareZkConnectionConfig =
- new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
- if (_realmAwareZkClientConfig == null) {
- _realmAwareZkClientConfig = new RealmAwareZkClient.RealmAwareZkClientConfig();
+ /**
+ * Creates a RealmAwareZkClient for ZkHelixClusterVerifiers.
+ * Note that DedicatedZkClient is used whether it's multi-realm or single-realm.
+ * @return
+ */
+ private RealmAwareZkClient createZkClientFromBuilderForVerifier() {
Review comment:
Turns out we don't need to override that here. This createZkClient() is private and has different logic for verifiers. Just to keep it clear that this logic is different from the regular createZkClient, I will keep the method name as is. Again, I'd like to emphasize that there's no extra public method so we are not adding anything to the interface.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r393846480
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -142,22 +142,22 @@ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
}
private ZkBaseDataAccessor(Builder<T> builder) {
- switch (builder.realmMode) {
+ switch (builder.getRealmMode()) {
case MULTI_REALM:
try {
- if (builder.zkClientType == ZkClientType.DEDICATED) {
+ if (builder._zkClientType == ZkClientType.DEDICATED) {
Review comment:
Following the previous comment, I can see this zkClientType is an exception which is not common for all the GenericZkHelixApiBuilder child classes' logic. In this case, we can let the ZkBaseDataAccessor builder extends my proposed GenericZkHelixApiBuilder method which creates a ZkClient, and in this extended method, we read and apply this _zkClientType.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r393827339
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -932,73 +933,13 @@ public void close() {
}
}
- public static class Builder {
- private String _zkAddress;
- private RealmAwareZkClient.RealmMode _realmMode;
- private RealmAwareZkClient.RealmAwareZkConnectionConfig _realmAwareZkConnectionConfig;
- private RealmAwareZkClient.RealmAwareZkClientConfig _realmAwareZkClientConfig;
-
+ public static class Builder extends GenericZkHelixApiBuilder<Builder> {
Review comment:
nit, this is more a question, I think you have thought about this. Shall we call it Generic**HelixZk**ApiBuilder?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395450703
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/GenericZkHelixApiBuilder.java
##########
@@ -0,0 +1,142 @@
+package org.apache.helix.manager.zk;
+
+/*
+ * 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 java.io.IOException;
+
+import org.apache.helix.HelixException;
+import org.apache.helix.msdcommon.exception.InvalidRoutingDataException;
+import org.apache.helix.zookeeper.api.client.HelixZkClient;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer;
+import org.apache.helix.zookeeper.impl.client.FederatedZkClient;
+import org.apache.helix.zookeeper.impl.factory.SharedZkClientFactory;
+
+
+/**
+ * GenericZkHelixApiBuilder serves as the abstract parent class for Builders used by Helix Java APIs
+ * that create ZK connections. By having this class, we reduce duplicate code as much as possible.
+ * @param <B>
+ */
+public abstract class GenericZkHelixApiBuilder<B extends GenericZkHelixApiBuilder<B>> {
Review comment:
Just to put some details of what we discussed, the only benefit of adding the build() method to the parent builder class is more complete interface design. But as Hunter mentioned, it is optional. I don't have a strong preference on this change, we will have the build() method for all the non-abstract builders anyway.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395351076
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/GenericZkHelixApiBuilder.java
##########
@@ -0,0 +1,142 @@
+package org.apache.helix.manager.zk;
+
+/*
+ * 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 java.io.IOException;
+
+import org.apache.helix.HelixException;
+import org.apache.helix.msdcommon.exception.InvalidRoutingDataException;
+import org.apache.helix.zookeeper.api.client.HelixZkClient;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer;
+import org.apache.helix.zookeeper.impl.client.FederatedZkClient;
+import org.apache.helix.zookeeper.impl.factory.SharedZkClientFactory;
+
+
+/**
+ * GenericZkHelixApiBuilder serves as the abstract parent class for Builders used by Helix Java APIs
+ * that create ZK connections. By having this class, we reduce duplicate code as much as possible.
+ * @param <B>
+ */
+public abstract class GenericZkHelixApiBuilder<B extends GenericZkHelixApiBuilder<B>> {
+ protected String _zkAddress;
+ protected RealmAwareZkClient.RealmMode _realmMode;
+ protected RealmAwareZkClient.RealmAwareZkConnectionConfig _realmAwareZkConnectionConfig;
+ protected RealmAwareZkClient.RealmAwareZkClientConfig _realmAwareZkClientConfig;
+
+ public B setZkAddress(String zkAddress) {
+ _zkAddress = zkAddress;
+ return self();
+ }
+
+ public B setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ _realmMode = realmMode;
+ return self();
+ }
+
+ public B setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ _realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return self();
+ }
+
+ public RealmAwareZkClient.RealmAwareZkConnectionConfig getRealmAwareZkConnectionConfig() {
+ return _realmAwareZkConnectionConfig;
+ }
+
+ public B setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ _realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return self();
+ }
+
+ /**
+ * Validates the given Builder parameters using a generic validation logic.
+ */
+ protected void validate() {
+ // Resolve RealmMode based on whether ZK address has been set
+ boolean isZkAddressSet = _zkAddress != null && !_zkAddress.isEmpty();
+ if (_realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM && !isZkAddressSet) {
+ throw new HelixException("RealmMode cannot be single-realm without a valid ZkAddress set!");
+ }
+ if (_realmMode == RealmAwareZkClient.RealmMode.MULTI_REALM && isZkAddressSet) {
+ throw new HelixException("ZkAddress cannot be set on multi-realm mode!");
+ }
+ if (_realmMode == null) {
+ _realmMode = isZkAddressSet ? RealmAwareZkClient.RealmMode.SINGLE_REALM
+ : RealmAwareZkClient.RealmMode.MULTI_REALM;
+ }
+
+ initializeConfigsIfNull();
+ }
+
+ /**
+ * Initializes Realm-aware ZkConnection and ZkClient configs if they haven't been set.
+ */
+ protected void initializeConfigsIfNull() {
+ // Resolve all default values
+ if (_realmAwareZkConnectionConfig == null) {
+ _realmAwareZkConnectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
+ }
+
+ // For Helix APIs, ZNRecord should be the default data model
+ if (_realmAwareZkClientConfig == null) {
+ _realmAwareZkClientConfig = new RealmAwareZkClient.RealmAwareZkClientConfig()
+ .setZkSerializer(new ZNRecordSerializer());
+ }
+ }
+
+ /**
+ * Creates a RealmAwareZkClient based on the parameters set.
+ * To be used in Helix ZK APIs' constructors: ConfigAccessor, ClusterSetup, ZKHelixAdmin
+ * @return
+ */
+ public RealmAwareZkClient createZkClientFromBuilder() {
Review comment:
I think just call it createZkClient() or createRealmAwareZkClient would be good enough.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r393855226
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -932,73 +933,13 @@ public void close() {
}
}
- public static class Builder {
- private String _zkAddress;
- private RealmAwareZkClient.RealmMode _realmMode;
- private RealmAwareZkClient.RealmAwareZkConnectionConfig _realmAwareZkConnectionConfig;
- private RealmAwareZkClient.RealmAwareZkClientConfig _realmAwareZkClientConfig;
-
+ public static class Builder extends GenericZkHelixApiBuilder<Builder> {
Review comment:
I am not so particular about the order of Helix and Zk. If you think HelixZk is more appropriate, we could do that. But note that there are existing APIs with the "ZkHelix" prefixes: ZkBaseDataAccessor, ZkHelixDataAccessor, ZkHeilxManager, etc.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395355148
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -87,26 +88,7 @@
* @param builder
*/
private ConfigAccessor(Builder builder) {
Review comment:
I tried to compare this solution with the other method, a private constructor accepts all parameters including the ZkClient.
Pros: less parameter.
Cons: you have to make createZkClient() method public.
Since the constructor is private and easy to update, will the option B be better? Moreover, many classes have it's defined constructors so you don't need to define a new one.
In this way, the builder will be very clean. Basically, the only public methods are setXXX() and build(). All the other methods are not required to be public. What do you think?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r394790401
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -87,11 +88,11 @@
* @param builder
*/
private ConfigAccessor(Builder builder) {
- switch (builder._realmMode) {
+ switch (builder.getRealmMode()) {
Review comment:
Moved all construction logic to the abstract builder, and refactored the code out of these Java API classes.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395342623
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/GenericZkHelixApiBuilder.java
##########
@@ -0,0 +1,142 @@
+package org.apache.helix.manager.zk;
+
+/*
+ * 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 java.io.IOException;
+
+import org.apache.helix.HelixException;
+import org.apache.helix.msdcommon.exception.InvalidRoutingDataException;
+import org.apache.helix.zookeeper.api.client.HelixZkClient;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer;
+import org.apache.helix.zookeeper.impl.client.FederatedZkClient;
+import org.apache.helix.zookeeper.impl.factory.SharedZkClientFactory;
+
+
+/**
+ * GenericZkHelixApiBuilder serves as the abstract parent class for Builders used by Helix Java APIs
+ * that create ZK connections. By having this class, we reduce duplicate code as much as possible.
+ * @param <B>
+ */
+public abstract class GenericZkHelixApiBuilder<B extends GenericZkHelixApiBuilder<B>> {
+ protected String _zkAddress;
+ protected RealmAwareZkClient.RealmMode _realmMode;
+ protected RealmAwareZkClient.RealmAwareZkConnectionConfig _realmAwareZkConnectionConfig;
+ protected RealmAwareZkClient.RealmAwareZkClientConfig _realmAwareZkClientConfig;
+
+ public B setZkAddress(String zkAddress) {
+ _zkAddress = zkAddress;
+ return self();
+ }
+
+ public B setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ _realmMode = realmMode;
+ return self();
+ }
+
+ public B setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ _realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return self();
+ }
+
+ public RealmAwareZkClient.RealmAwareZkConnectionConfig getRealmAwareZkConnectionConfig() {
+ return _realmAwareZkConnectionConfig;
+ }
+
+ public B setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ _realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return self();
+ }
+
+ /**
+ * Validates the given Builder parameters using a generic validation logic.
+ */
+ protected void validate() {
+ // Resolve RealmMode based on whether ZK address has been set
+ boolean isZkAddressSet = _zkAddress != null && !_zkAddress.isEmpty();
+ if (_realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM && !isZkAddressSet) {
+ throw new HelixException("RealmMode cannot be single-realm without a valid ZkAddress set!");
+ }
+ if (_realmMode == RealmAwareZkClient.RealmMode.MULTI_REALM && isZkAddressSet) {
+ throw new HelixException("ZkAddress cannot be set on multi-realm mode!");
+ }
+ if (_realmMode == null) {
+ _realmMode = isZkAddressSet ? RealmAwareZkClient.RealmMode.SINGLE_REALM
+ : RealmAwareZkClient.RealmMode.MULTI_REALM;
+ }
+
+ initializeConfigsIfNull();
+ }
+
+ /**
+ * Initializes Realm-aware ZkConnection and ZkClient configs if they haven't been set.
+ */
+ protected void initializeConfigsIfNull() {
+ // Resolve all default values
+ if (_realmAwareZkConnectionConfig == null) {
+ _realmAwareZkConnectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
+ }
+
+ // For Helix APIs, ZNRecord should be the default data model
+ if (_realmAwareZkClientConfig == null) {
+ _realmAwareZkClientConfig = new RealmAwareZkClient.RealmAwareZkClientConfig()
+ .setZkSerializer(new ZNRecordSerializer());
+ }
+ }
+
+ /**
+ * Creates a RealmAwareZkClient based on the parameters set.
+ * To be used in Helix ZK APIs' constructors: ConfigAccessor, ClusterSetup, ZKHelixAdmin
+ * @return
+ */
+ public RealmAwareZkClient createZkClientFromBuilder() {
Review comment:
Is it intended to be done with a different name in the parent class? Note in the child builder it is called createRealmAwareZkClientFromBuilder().
One of the reasons that I want to use the same name is avoiding the caller to be confused when they have more than one method to call when they try to get a ZkClient instance from the builder.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on issue #899: Use Java Generics and
inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on issue #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#issuecomment-601838962
> > Looks good in general.
> > The only thing concerns me is the newly modified deprecated constructors. We should either keep the same behavior as the master branch and then mark them as deprecated, or change them to use the new builder logic completely. The current diff on the zooscalability branch is mixed.
> > Deprecating them would be easier and safer. But we don't need to touch them in this PR.
> > So can we have a small follow-up PR to:
> >
> > 1. revert the newly modified constructors that are not onboarding to the new builder. Since builder should be the new way we create an accessor/verifier that uses a realm-aware ZkClient, right?
> > 2. mark the old constructor deprecated.
>
> This has been discussed offline among other PMC and committers, and we have considered this possibility, but we decided that in order to make rollout and adoption smoother, we have opted for this design. This way, users can easily test out the behavior without having to make any code change. Does that make sense to you?
>
> The behavior in fact is exactly same. The difference kicks in only if the user sets the multiZk System config. Also note that the end-user who uses Helix Java APIs do not know what kind of zkclient is returned - it is an internal detail that belongs inside Java API construction logic.
>
> As far as making a new method deprecated, I think there's a tradeoff here. I do not have to deprecate it if you're not comfortable with it, but the method itself is needed in order to promote code reuse. It's just the logic that should be deprecated.
I discussed with Hunter offline, design-wise, I'm totally fine. But implementation-wise, we do have 2 options here. 1. Creating a builder might be overkill because it is for backward compatibility only. 2. On the other hand, my concern for the current design is that, if we have multiple zkclient user classes have such kind usage, we will need to do if-else check based on the system property MULTI_ZK_ENABLED multiple times. And that property is also for backward compatibility only, which will be eventually removed. There is a trade-off we need to consider.
But, as I commented before, there is no need to be blocked on this topic in this PR.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395343695
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -260,10 +217,9 @@ public ZkBaseDataAccessor(String zkAddress, ZkClientType zkClientType) {
@Deprecated
public ZkBaseDataAccessor(String zkAddress, ZkSerializer zkSerializer,
ZkClientType zkClientType) {
- RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
- new RealmAwareZkClient.RealmAwareZkClientConfig().setZkSerializer(zkSerializer);
-
- _zkClient = buildRealmAwareZkClient(clientConfig, zkAddress, zkClientType);
+ _zkClient = buildRealmAwareZkClientWithDefaultConfigs(
Review comment:
I would prefer to leverage builder here as well. So we can merge the logic of buildRealmAwareZkClientWithDefaultConfigs into the builder.
Not sure if there are any conflicts. If there are, I don't have a strong preference that we have to do this change.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395344143
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -126,41 +117,9 @@ public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String
public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath,
List<String> wtCachePaths, List<String> zkCachePaths, String monitorType, String monitorkey,
ZkBaseDataAccessor.ZkClientType zkClientType) {
-
- // If the multi ZK config is enabled, use multi-realm mode with FederatedZkClient
- if (Boolean.parseBoolean(System.getProperty(SystemPropertyKeys.MULTI_ZK_ENABLED))) {
- try {
- RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder connectionConfigBuilder =
- new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder();
- RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
- new RealmAwareZkClient.RealmAwareZkClientConfig();
- clientConfig.setZkSerializer(serializer).setMonitorType(monitorType)
- .setMonitorKey(monitorkey);
- // Use a federated zk client
- _zkClient = new FederatedZkClient(connectionConfigBuilder.build(), clientConfig);
- } catch (IOException | InvalidRoutingDataException | IllegalStateException e) {
- // Note: IllegalStateException is for HttpRoutingDataReader if MSDS endpoint cannot be
- // found
- throw new HelixException("Failed to create ZkCacheBaseDataAccessor!", e);
- }
- } else {
- HelixZkClient.ZkClientConfig clientConfig = new HelixZkClient.ZkClientConfig();
- clientConfig.setZkSerializer(serializer).setMonitorType(monitorType)
- .setMonitorKey(monitorkey);
- switch (zkClientType) {
- case DEDICATED:
- _zkClient = DedicatedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress),
- new HelixZkClient.ZkClientConfig().setZkSerializer(serializer));
- break;
- case SHARED:
- default:
- _zkClient = SharedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(zkAddress), clientConfig);
- }
- _zkClient.waitUntilConnected(HelixZkClient.DEFAULT_CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS);
- }
-
+ _zkClient = ZkBaseDataAccessor.buildRealmAwareZkClientWithDefaultConfigs(
Review comment:
Same 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly merged pull request #899: Use Java Generics and
inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly merged pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on issue #899: Use Java Generics and
inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on issue #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#issuecomment-601842437
> > > Looks good in general.
> > > The only thing concerns me is the newly modified deprecated constructors. We should either keep the same behavior as the master branch and then mark them as deprecated, or change them to use the new builder logic completely. The current diff on the zooscalability branch is mixed.
> > > Deprecating them would be easier and safer. But we don't need to touch them in this PR.
> > > So can we have a small follow-up PR to:
> > >
> > > 1. revert the newly modified constructors that are not onboarding to the new builder. Since builder should be the new way we create an accessor/verifier that uses a realm-aware ZkClient, right?
> > > 2. mark the old constructor deprecated.
> >
> >
> > This has been discussed offline among other PMC and committers, and we have considered this possibility, but we decided that in order to make rollout and adoption smoother, we have opted for this design. This way, users can easily test out the behavior without having to make any code change. Does that make sense to you?
> > The behavior in fact is exactly same. The difference kicks in only if the user sets the multiZk System config. Also note that the end-user who uses Helix Java APIs do not know what kind of zkclient is returned - it is an internal detail that belongs inside Java API construction logic.
> > As far as making a new method deprecated, I think there's a tradeoff here. I do not have to deprecate it if you're not comfortable with it, but the method itself is needed in order to promote code reuse. It's just the logic that should be deprecated.
>
> I discussed with Hunter offline, design-wise, I'm totally fine. But implementation-wise, we do have 2 options here. 1. Creating a builder might be overkill because it is for backward compatibility only. 2. On the other hand, my concern for the current design is that, if we have multiple zkclient user classes have such kind usage, we will need to do if-else check based on the system property MULTI_ZK_ENABLED multiple times. And that property is also for backward compatibility only, which will be eventually removed. There is a trade-off we need to consider.
> But, as I commented before, there is no need to be blocked on this topic in this PR.
Thanks for summarizing our discussion. In terms of your last point, let me also add that I am not a big fan of the idea of creating a Builder just to make things backward-compatible because 1) it doesn't reduce duplicate code (in fact, it may end up adding more boilerplate code) 2) there is no clear common logic other than reading from System.Properties, and 3) I'd argue that sometimes a simple if-else does the trick and is the easiest to understand.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395348149
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/GenericZkHelixApiBuilder.java
##########
@@ -0,0 +1,142 @@
+package org.apache.helix.manager.zk;
+
+/*
+ * 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 java.io.IOException;
+
+import org.apache.helix.HelixException;
+import org.apache.helix.msdcommon.exception.InvalidRoutingDataException;
+import org.apache.helix.zookeeper.api.client.HelixZkClient;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer;
+import org.apache.helix.zookeeper.impl.client.FederatedZkClient;
+import org.apache.helix.zookeeper.impl.factory.SharedZkClientFactory;
+
+
+/**
+ * GenericZkHelixApiBuilder serves as the abstract parent class for Builders used by Helix Java APIs
+ * that create ZK connections. By having this class, we reduce duplicate code as much as possible.
+ * @param <B>
+ */
+public abstract class GenericZkHelixApiBuilder<B extends GenericZkHelixApiBuilder<B>> {
Review comment:
Shall we include the target class as a template T in addition to B here? So you can define the abstract build() method to make this class more complete.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395812800
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -384,14 +373,26 @@ protected void validate() {
+ _zkAddress + " RealmAwareZkConnectionConfig: " + _realmAwareZkConnectionConfig);
}
}
+ initializeConfigsIfNull();
+ }
- // Resolve all default values
- if (_realmAwareZkConnectionConfig == null) {
- _realmAwareZkConnectionConfig =
- new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
- if (_realmAwareZkClientConfig == null) {
- _realmAwareZkClientConfig = new RealmAwareZkClient.RealmAwareZkClientConfig();
+ /**
+ * Creates a RealmAwareZkClient for ZkHelixClusterVerifiers.
+ * Note that DedicatedZkClient is used whether it's multi-realm or single-realm.
+ * @return
+ */
+ private RealmAwareZkClient createZkClientFromBuilderForVerifier() {
Review comment:
This is not quite about the public interface. The ZkClient build behavior of this builder should be this logic, right? So we should override the method here. Unless we do have a use case of this specific builder that requires to call the parent class createZkClient(). Otherwise, it just causes confusing IMO.
What is your concern not doing this?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395348347
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/GenericZkHelixApiBuilder.java
##########
@@ -0,0 +1,142 @@
+package org.apache.helix.manager.zk;
+
+/*
+ * 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 java.io.IOException;
+
+import org.apache.helix.HelixException;
+import org.apache.helix.msdcommon.exception.InvalidRoutingDataException;
+import org.apache.helix.zookeeper.api.client.HelixZkClient;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer;
+import org.apache.helix.zookeeper.impl.client.FederatedZkClient;
+import org.apache.helix.zookeeper.impl.factory.SharedZkClientFactory;
+
+
+/**
+ * GenericZkHelixApiBuilder serves as the abstract parent class for Builders used by Helix Java APIs
+ * that create ZK connections. By having this class, we reduce duplicate code as much as possible.
+ * @param <B>
+ */
+public abstract class GenericZkHelixApiBuilder<B extends GenericZkHelixApiBuilder<B>> {
+ protected String _zkAddress;
+ protected RealmAwareZkClient.RealmMode _realmMode;
+ protected RealmAwareZkClient.RealmAwareZkConnectionConfig _realmAwareZkConnectionConfig;
+ protected RealmAwareZkClient.RealmAwareZkClientConfig _realmAwareZkClientConfig;
+
+ public B setZkAddress(String zkAddress) {
+ _zkAddress = zkAddress;
+ return self();
+ }
+
+ public B setRealmMode(RealmAwareZkClient.RealmMode realmMode) {
+ _realmMode = realmMode;
+ return self();
+ }
+
+ public B setRealmAwareZkConnectionConfig(
+ RealmAwareZkClient.RealmAwareZkConnectionConfig realmAwareZkConnectionConfig) {
+ _realmAwareZkConnectionConfig = realmAwareZkConnectionConfig;
+ return self();
+ }
+
+ public RealmAwareZkClient.RealmAwareZkConnectionConfig getRealmAwareZkConnectionConfig() {
+ return _realmAwareZkConnectionConfig;
+ }
+
+ public B setRealmAwareZkClientConfig(
+ RealmAwareZkClient.RealmAwareZkClientConfig realmAwareZkClientConfig) {
+ _realmAwareZkClientConfig = realmAwareZkClientConfig;
+ return self();
+ }
+
+ /**
+ * Validates the given Builder parameters using a generic validation logic.
+ */
+ protected void validate() {
+ // Resolve RealmMode based on whether ZK address has been set
+ boolean isZkAddressSet = _zkAddress != null && !_zkAddress.isEmpty();
+ if (_realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM && !isZkAddressSet) {
+ throw new HelixException("RealmMode cannot be single-realm without a valid ZkAddress set!");
+ }
+ if (_realmMode == RealmAwareZkClient.RealmMode.MULTI_REALM && isZkAddressSet) {
+ throw new HelixException("ZkAddress cannot be set on multi-realm mode!");
+ }
+ if (_realmMode == null) {
+ _realmMode = isZkAddressSet ? RealmAwareZkClient.RealmMode.SINGLE_REALM
+ : RealmAwareZkClient.RealmMode.MULTI_REALM;
+ }
+
+ initializeConfigsIfNull();
+ }
+
+ /**
+ * Initializes Realm-aware ZkConnection and ZkClient configs if they haven't been set.
+ */
+ protected void initializeConfigsIfNull() {
+ // Resolve all default values
+ if (_realmAwareZkConnectionConfig == null) {
+ _realmAwareZkConnectionConfig =
+ new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
+ }
+
+ // For Helix APIs, ZNRecord should be the default data model
+ if (_realmAwareZkClientConfig == null) {
+ _realmAwareZkClientConfig = new RealmAwareZkClient.RealmAwareZkClientConfig()
+ .setZkSerializer(new ZNRecordSerializer());
+ }
+ }
+
+ /**
+ * Creates a RealmAwareZkClient based on the parameters set.
+ * To be used in Helix ZK APIs' constructors: ConfigAccessor, ClusterSetup, ZKHelixAdmin
+ * @return
+ */
+ public RealmAwareZkClient createZkClientFromBuilder() {
+ switch (_realmMode) {
+ case MULTI_REALM:
+ try {
+ return new FederatedZkClient(_realmAwareZkConnectionConfig,
+ _realmAwareZkClientConfig.setZkSerializer(new ZNRecordSerializer()));
+ } catch (IOException | InvalidRoutingDataException | IllegalStateException e) {
+ throw new HelixException("Failed to create FederatedZkClient!", e);
+ }
+ case SINGLE_REALM:
+ // Create a HelixZkClient: Use a SharedZkClient because ClusterSetup does not need to do
+ // ephemeral operations
+ return SharedZkClientFactory.getInstance()
+ .buildZkClient(new HelixZkClient.ZkConnectionConfig(_zkAddress),
+ _realmAwareZkClientConfig.createHelixZkClientConfig()
+ .setZkSerializer(new ZNRecordSerializer()));
+ default:
+ throw new HelixException("Invalid RealmMode given: " + _realmMode);
+ }
+ }
+
+ /**
+ * Returns an instance of a subclass-Builder in order to reduce duplicate code.
+ * SuppressWarnings is used to rid of IDE warnings.
+ * @return an instance of a subclass-Builder. E.g.) ConfigAccessor.Builder
+ */
+ @SuppressWarnings("unchecked")
+ final B self() {
Review comment:
This looks nice : )
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395346538
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/ZkClientType.java
##########
@@ -0,0 +1,26 @@
+package org.apache.helix.zookeeper.api.client;
+
+/*
+ * 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 enum ZkClientType {
Review comment:
More java doc to specify what's the difference between types, please.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395734888
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -126,41 +117,9 @@ public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String
public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath,
Review comment:
I will deprecate this but I do not agree with that we should revert the behavior.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r393844493
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -142,22 +142,22 @@ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
}
private ZkBaseDataAccessor(Builder<T> builder) {
- switch (builder.realmMode) {
+ switch (builder.getRealmMode()) {
Review comment:
The following logic looks generic enough to put in GenericZkHelixApiBuilder. And this ZkBaseDataAccessor constructor can directly get a zkclient instance as the input.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395806370
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -1390,65 +1311,23 @@ public Builder() {
validate();
return new ZkBaseDataAccessor<>(this);
}
-
- /*
- * Validates the given parameters before building an instance of ZkBaseDataAccessor.
- */
- private void validate() {
- // Resolve RealmMode based on other parameters
- boolean isZkAddressSet = zkAddress != null && !zkAddress.isEmpty();
- boolean isZkClientTypeSet = zkClientType != null;
-
- // If ZkClientType is set, RealmMode must either be single-realm or not set.
- if (isZkClientTypeSet && realmMode == RealmAwareZkClient.RealmMode.MULTI_REALM) {
- throw new HelixException("ZkClientType cannot be set on multi-realm mode!");
- }
- // If ZkClientType is not set and realmMode is single-realm, default to SHARED
- if (!isZkClientTypeSet && realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM) {
- zkClientType = ZkClientType.SHARED;
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM && !isZkAddressSet) {
- throw new HelixException("RealmMode cannot be single-realm without a valid ZkAddress set!");
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.MULTI_REALM && isZkAddressSet) {
- throw new HelixException("ZkAddress cannot be set on multi-realm mode!");
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM
- && zkClientType == ZkClientType.FEDERATED) {
- throw new HelixException("FederatedZkClient cannot be set on single-realm mode!");
- }
-
- if (realmMode == null) {
- realmMode = isZkAddressSet ? RealmAwareZkClient.RealmMode.SINGLE_REALM
- : RealmAwareZkClient.RealmMode.MULTI_REALM;
- }
-
- // Resolve RealmAwareZkClientConfig
- if (realmAwareZkClientConfig == null) {
- realmAwareZkClientConfig = new RealmAwareZkClient.RealmAwareZkClientConfig()
- .setZkSerializer(new ZNRecordSerializer());
- }
-
- // Resolve RealmAwareZkConnectionConfig
- if (realmAwareZkConnectionConfig == null) {
- // If not set, create a default one
- realmAwareZkConnectionConfig =
- new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
- }
}
- /*
- * This is used for constructors that do not take a Builder in as a parameter because of
- * keeping backward-compatibility.
+ /**
+ * This method is used for constructors that are not based on the Builder for
+ * backward-compatibility.
+ * It checks if there is a System Property config set for Multi-ZK mode and determines if a
+ * FederatedZkClient should be created.
+ * @param clientConfig default RealmAwareZkClientConfig with ZK serializer set
+ * @param zkAddress
+ * @param zkClientType
+ * @return
*/
- private RealmAwareZkClient buildRealmAwareZkClient(
+ static RealmAwareZkClient buildRealmAwareZkClientWithDefaultConfigs(
Review comment:
Discussed offline.
As for deprecating the new little method stub, I will remove the deprecated annotation, but i think we need to look at it as an improvement we are making to the existing code because this is **not** newly added logic. we have refactored it to cut down on duplicate code. The method stub is created solely for that purpose.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395391736
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/GenericZkHelixApiBuilder.java
##########
@@ -0,0 +1,142 @@
+package org.apache.helix.manager.zk;
+
+/*
+ * 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 java.io.IOException;
+
+import org.apache.helix.HelixException;
+import org.apache.helix.msdcommon.exception.InvalidRoutingDataException;
+import org.apache.helix.zookeeper.api.client.HelixZkClient;
+import org.apache.helix.zookeeper.api.client.RealmAwareZkClient;
+import org.apache.helix.zookeeper.datamodel.serializer.ZNRecordSerializer;
+import org.apache.helix.zookeeper.impl.client.FederatedZkClient;
+import org.apache.helix.zookeeper.impl.factory.SharedZkClientFactory;
+
+
+/**
+ * GenericZkHelixApiBuilder serves as the abstract parent class for Builders used by Helix Java APIs
+ * that create ZK connections. By having this class, we reduce duplicate code as much as possible.
+ * @param <B>
+ */
+public abstract class GenericZkHelixApiBuilder<B extends GenericZkHelixApiBuilder<B>> {
Review comment:
Discussed offline. Doing this has no clear benefit because the return types (Helix APIs) do not have a hierarchical relationship or inheritance relationship. It also doesn't reduce duplicate code. We will keep things as is.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395452606
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -1390,65 +1311,23 @@ public Builder() {
validate();
return new ZkBaseDataAccessor<>(this);
}
-
- /*
- * Validates the given parameters before building an instance of ZkBaseDataAccessor.
- */
- private void validate() {
- // Resolve RealmMode based on other parameters
- boolean isZkAddressSet = zkAddress != null && !zkAddress.isEmpty();
- boolean isZkClientTypeSet = zkClientType != null;
-
- // If ZkClientType is set, RealmMode must either be single-realm or not set.
- if (isZkClientTypeSet && realmMode == RealmAwareZkClient.RealmMode.MULTI_REALM) {
- throw new HelixException("ZkClientType cannot be set on multi-realm mode!");
- }
- // If ZkClientType is not set and realmMode is single-realm, default to SHARED
- if (!isZkClientTypeSet && realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM) {
- zkClientType = ZkClientType.SHARED;
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM && !isZkAddressSet) {
- throw new HelixException("RealmMode cannot be single-realm without a valid ZkAddress set!");
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.MULTI_REALM && isZkAddressSet) {
- throw new HelixException("ZkAddress cannot be set on multi-realm mode!");
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM
- && zkClientType == ZkClientType.FEDERATED) {
- throw new HelixException("FederatedZkClient cannot be set on single-realm mode!");
- }
-
- if (realmMode == null) {
- realmMode = isZkAddressSet ? RealmAwareZkClient.RealmMode.SINGLE_REALM
- : RealmAwareZkClient.RealmMode.MULTI_REALM;
- }
-
- // Resolve RealmAwareZkClientConfig
- if (realmAwareZkClientConfig == null) {
- realmAwareZkClientConfig = new RealmAwareZkClient.RealmAwareZkClientConfig()
- .setZkSerializer(new ZNRecordSerializer());
- }
-
- // Resolve RealmAwareZkConnectionConfig
- if (realmAwareZkConnectionConfig == null) {
- // If not set, create a default one
- realmAwareZkConnectionConfig =
- new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
- }
}
- /*
- * This is used for constructors that do not take a Builder in as a parameter because of
- * keeping backward-compatibility.
+ /**
+ * This method is used for constructors that are not based on the Builder for
+ * backward-compatibility.
+ * It checks if there is a System Property config set for Multi-ZK mode and determines if a
+ * FederatedZkClient should be created.
+ * @param clientConfig default RealmAwareZkClientConfig with ZK serializer set
+ * @param zkAddress
+ * @param zkClientType
+ * @return
*/
- private RealmAwareZkClient buildRealmAwareZkClient(
+ static RealmAwareZkClient buildRealmAwareZkClientWithDefaultConfigs(
Review comment:
There is no need to deprecate, we can directly remove it. I checked, it is not in the master branch yet.
Same for the other usages. I think we should just remove it instead of introducing a new but deprecated method to the master branch.
If you are worried about the backward compatibility, then we can just leave the very old code that creates either DEDICATED or SHARED client. Those constructors are deprecated anyway.
We don't need to change the logic of the deprecated methods to return RealmAware ZkClient. As you said, this will just mix the new logic with the old/deprecated ones.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r393882091
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -142,22 +142,22 @@ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
}
private ZkBaseDataAccessor(Builder<T> builder) {
- switch (builder.realmMode) {
+ switch (builder.getRealmMode()) {
case MULTI_REALM:
try {
- if (builder.zkClientType == ZkClientType.DEDICATED) {
+ if (builder._zkClientType == ZkClientType.DEDICATED) {
Review comment:
Just to summarize:
This is the construction logic that only pertains to ZkBaseDataAccessor/ZkCacheBaseDataAccessor
Your feedback was that this is duplicated in both ZkBaseDataAccessor and ZkCacheBaseDataAccessor
I agree with your observation
Now we have a few options in terms of how to refactor this logic
Make it a reusable method that pertains to data accessors
2. Put this in the Builder
Here, between 1 and 2, my preference is 1, because this seems like business logic that belongs to the data accessor, not the builder.
What do you think?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395346159
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -144,23 +145,11 @@ public ZkHelixClusterVerifier(String zkAddr, String clusterName) {
_keyBuilder = _accessor.keyBuilder();
}
- protected ZkHelixClusterVerifier(Builder builder) {
- if (Boolean.parseBoolean(System.getProperty(SystemPropertyKeys.MULTI_ZK_ENABLED))) {
- try {
- // First, try to create a RealmAwareZkClient that's a DedicatedZkClient
- _zkClient = DedicatedZkClientFactory.getInstance()
- .buildZkClient(builder._realmAwareZkConnectionConfig,
- builder._realmAwareZkClientConfig);
- } catch (IOException | InvalidRoutingDataException | IllegalStateException e) {
- throw new HelixException("ZkHelixClusterVerifier: failed to create ZkClient!", e);
- }
- } else {
- _zkClient = DedicatedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(builder._zkAddress));
- }
- _usesExternalZkClient = false;
+ protected <B extends Builder<B>> ZkHelixClusterVerifier(Builder<B> builder) {
+ _zkClient = builder.createZkClientFromBuilderForVerifier();
_zkClient.setZkSerializer(new ZNRecordSerializer());
- _clusterName = builder._realmAwareZkConnectionConfig.getZkRealmShardingKey();
+ _usesExternalZkClient = false;
+ _clusterName = builder.getRealmAwareZkConnectionConfig().getZkRealmShardingKey();
Review comment:
The reason we call the path as "shardingkey" is to make it more generic. If we assign it to the clusterName, it means we set a strong assumption here that for Helix, the shardingkey == clusterName. Although this assumption is valid, the code is confusing.
So shall we add an additional method builder.getClusterName() to avoid confusion? The internal logic is the same. Note that both builder classes are in Helix-core. So we should reduce exposing Realm concept if not necessary.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r394669071
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -87,11 +88,11 @@
* @param builder
*/
private ConfigAccessor(Builder builder) {
- switch (builder._realmMode) {
+ switch (builder.getRealmMode()) {
Review comment:
Isn't this switch block identical to the ones in ZKHelixAdmin and ClusterSetup? IMO, let the builder have a method for creating a ZkClient is simpler and easier to maintain.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395392136
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -1390,65 +1311,23 @@ public Builder() {
validate();
return new ZkBaseDataAccessor<>(this);
}
-
- /*
- * Validates the given parameters before building an instance of ZkBaseDataAccessor.
- */
- private void validate() {
- // Resolve RealmMode based on other parameters
- boolean isZkAddressSet = zkAddress != null && !zkAddress.isEmpty();
- boolean isZkClientTypeSet = zkClientType != null;
-
- // If ZkClientType is set, RealmMode must either be single-realm or not set.
- if (isZkClientTypeSet && realmMode == RealmAwareZkClient.RealmMode.MULTI_REALM) {
- throw new HelixException("ZkClientType cannot be set on multi-realm mode!");
- }
- // If ZkClientType is not set and realmMode is single-realm, default to SHARED
- if (!isZkClientTypeSet && realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM) {
- zkClientType = ZkClientType.SHARED;
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM && !isZkAddressSet) {
- throw new HelixException("RealmMode cannot be single-realm without a valid ZkAddress set!");
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.MULTI_REALM && isZkAddressSet) {
- throw new HelixException("ZkAddress cannot be set on multi-realm mode!");
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM
- && zkClientType == ZkClientType.FEDERATED) {
- throw new HelixException("FederatedZkClient cannot be set on single-realm mode!");
- }
-
- if (realmMode == null) {
- realmMode = isZkAddressSet ? RealmAwareZkClient.RealmMode.SINGLE_REALM
- : RealmAwareZkClient.RealmMode.MULTI_REALM;
- }
-
- // Resolve RealmAwareZkClientConfig
- if (realmAwareZkClientConfig == null) {
- realmAwareZkClientConfig = new RealmAwareZkClient.RealmAwareZkClientConfig()
- .setZkSerializer(new ZNRecordSerializer());
- }
-
- // Resolve RealmAwareZkConnectionConfig
- if (realmAwareZkConnectionConfig == null) {
- // If not set, create a default one
- realmAwareZkConnectionConfig =
- new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
- }
}
- /*
- * This is used for constructors that do not take a Builder in as a parameter because of
- * keeping backward-compatibility.
+ /**
+ * This method is used for constructors that are not based on the Builder for
+ * backward-compatibility.
+ * It checks if there is a System Property config set for Multi-ZK mode and determines if a
+ * FederatedZkClient should be created.
+ * @param clientConfig default RealmAwareZkClientConfig with ZK serializer set
+ * @param zkAddress
+ * @param zkClientType
+ * @return
*/
- private RealmAwareZkClient buildRealmAwareZkClient(
+ static RealmAwareZkClient buildRealmAwareZkClientWithDefaultConfigs(
Review comment:
I deprecated this method. this is old construction logic, so let's not try to mix it with our new construction logic in the builder :)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r394790262
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -142,22 +142,22 @@ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
}
private ZkBaseDataAccessor(Builder<T> builder) {
- switch (builder.realmMode) {
+ switch (builder.getRealmMode()) {
case MULTI_REALM:
try {
- if (builder.zkClientType == ZkClientType.DEDICATED) {
+ if (builder._zkClientType == ZkClientType.DEDICATED) {
Review comment:
I value your opinion and it also makes sense. For now, I can move all construction logic to the builders. I have updated the PR accordingly.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r393847317
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -142,22 +142,22 @@ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
}
private ZkBaseDataAccessor(Builder<T> builder) {
- switch (builder.realmMode) {
+ switch (builder.getRealmMode()) {
case MULTI_REALM:
try {
- if (builder.zkClientType == ZkClientType.DEDICATED) {
+ if (builder._zkClientType == ZkClientType.DEDICATED) {
Review comment:
Or, if there are more than one builders require zkClientType, we can have another generic builder which includes the zkClientType logic and let it to extend GenericZkHelixApiBuilder.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395392380
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -144,23 +145,11 @@ public ZkHelixClusterVerifier(String zkAddr, String clusterName) {
_keyBuilder = _accessor.keyBuilder();
}
- protected ZkHelixClusterVerifier(Builder builder) {
- if (Boolean.parseBoolean(System.getProperty(SystemPropertyKeys.MULTI_ZK_ENABLED))) {
- try {
- // First, try to create a RealmAwareZkClient that's a DedicatedZkClient
- _zkClient = DedicatedZkClientFactory.getInstance()
- .buildZkClient(builder._realmAwareZkConnectionConfig,
- builder._realmAwareZkClientConfig);
- } catch (IOException | InvalidRoutingDataException | IllegalStateException e) {
- throw new HelixException("ZkHelixClusterVerifier: failed to create ZkClient!", e);
- }
- } else {
- _zkClient = DedicatedZkClientFactory.getInstance()
- .buildZkClient(new HelixZkClient.ZkConnectionConfig(builder._zkAddress));
- }
- _usesExternalZkClient = false;
+ protected <B extends Builder<B>> ZkHelixClusterVerifier(Builder<B> builder) {
+ _zkClient = builder.createZkClientFromBuilderForVerifier();
_zkClient.setZkSerializer(new ZNRecordSerializer());
- _clusterName = builder._realmAwareZkConnectionConfig.getZkRealmShardingKey();
+ _usesExternalZkClient = false;
+ _clusterName = builder.getRealmAwareZkConnectionConfig().getZkRealmShardingKey();
Review comment:
This is actually also on my mind. Thanks for pointing this out - we are on the same page. I will add getClusterName() to avoid confusion.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395391922
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -260,10 +217,9 @@ public ZkBaseDataAccessor(String zkAddress, ZkClientType zkClientType) {
@Deprecated
public ZkBaseDataAccessor(String zkAddress, ZkSerializer zkSerializer,
ZkClientType zkClientType) {
- RealmAwareZkClient.RealmAwareZkClientConfig clientConfig =
- new RealmAwareZkClient.RealmAwareZkClientConfig().setZkSerializer(zkSerializer);
-
- _zkClient = buildRealmAwareZkClient(clientConfig, zkAddress, zkClientType);
+ _zkClient = buildRealmAwareZkClientWithDefaultConfigs(
Review comment:
Discussed offline. This buildRealmAwareZkClientWithDefaultConfigs logic actually belongs to an existing, old construction logic, so this shouldn't go into the Builder.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395846744
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -384,14 +373,26 @@ protected void validate() {
+ _zkAddress + " RealmAwareZkConnectionConfig: " + _realmAwareZkConnectionConfig);
}
}
+ initializeConfigsIfNull();
+ }
- // Resolve all default values
- if (_realmAwareZkConnectionConfig == null) {
- _realmAwareZkConnectionConfig =
- new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
- if (_realmAwareZkClientConfig == null) {
- _realmAwareZkClientConfig = new RealmAwareZkClient.RealmAwareZkClientConfig();
+ /**
+ * Creates a RealmAwareZkClient for ZkHelixClusterVerifiers.
+ * Note that DedicatedZkClient is used whether it's multi-realm or single-realm.
+ * @return
+ */
+ private RealmAwareZkClient createZkClientFromBuilderForVerifier() {
Review comment:
@jiajunwang Because this would cause the constructor to take in a list of parameters defined by its parent builder's createZkClient(). I'd argue that it's not confusing because the method name makes is 100% clear - there really is no ambiguity.
For ZkHelixClusterVerifier and its implementations, they, by definition, only operate on single realm mode on a dedicated zk client. As such, that degree of customization that would be required in the parent builder's createZkClient() is not necessary.
Is it a good idea to have unnecessary parameters in a method just for the sake of overriding a parent method, especially when it could be avoided?
Again, this is a stylistic choice, and for the sake of moving this forward, I will make that change to make the method override its parent's method - that way we would be on the same page.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395344000
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -1390,65 +1311,23 @@ public Builder() {
validate();
return new ZkBaseDataAccessor<>(this);
}
-
- /*
- * Validates the given parameters before building an instance of ZkBaseDataAccessor.
- */
- private void validate() {
- // Resolve RealmMode based on other parameters
- boolean isZkAddressSet = zkAddress != null && !zkAddress.isEmpty();
- boolean isZkClientTypeSet = zkClientType != null;
-
- // If ZkClientType is set, RealmMode must either be single-realm or not set.
- if (isZkClientTypeSet && realmMode == RealmAwareZkClient.RealmMode.MULTI_REALM) {
- throw new HelixException("ZkClientType cannot be set on multi-realm mode!");
- }
- // If ZkClientType is not set and realmMode is single-realm, default to SHARED
- if (!isZkClientTypeSet && realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM) {
- zkClientType = ZkClientType.SHARED;
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM && !isZkAddressSet) {
- throw new HelixException("RealmMode cannot be single-realm without a valid ZkAddress set!");
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.MULTI_REALM && isZkAddressSet) {
- throw new HelixException("ZkAddress cannot be set on multi-realm mode!");
- }
-
- if (realmMode == RealmAwareZkClient.RealmMode.SINGLE_REALM
- && zkClientType == ZkClientType.FEDERATED) {
- throw new HelixException("FederatedZkClient cannot be set on single-realm mode!");
- }
-
- if (realmMode == null) {
- realmMode = isZkAddressSet ? RealmAwareZkClient.RealmMode.SINGLE_REALM
- : RealmAwareZkClient.RealmMode.MULTI_REALM;
- }
-
- // Resolve RealmAwareZkClientConfig
- if (realmAwareZkClientConfig == null) {
- realmAwareZkClientConfig = new RealmAwareZkClient.RealmAwareZkClientConfig()
- .setZkSerializer(new ZNRecordSerializer());
- }
-
- // Resolve RealmAwareZkConnectionConfig
- if (realmAwareZkConnectionConfig == null) {
- // If not set, create a default one
- realmAwareZkConnectionConfig =
- new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
- }
}
- /*
- * This is used for constructors that do not take a Builder in as a parameter because of
- * keeping backward-compatibility.
+ /**
+ * This method is used for constructors that are not based on the Builder for
+ * backward-compatibility.
+ * It checks if there is a System Property config set for Multi-ZK mode and determines if a
+ * FederatedZkClient should be created.
+ * @param clientConfig default RealmAwareZkClientConfig with ZK serializer set
+ * @param zkAddress
+ * @param zkClientType
+ * @return
*/
- private RealmAwareZkClient buildRealmAwareZkClient(
+ static RealmAwareZkClient buildRealmAwareZkClientWithDefaultConfigs(
Review comment:
Not sure if we can merge it to the builder logic? If not possible for now, let's have a TODO 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395453530
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -126,41 +117,9 @@ public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String
public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath,
Review comment:
This one shall be deprecated as well. And I would suggest keeping the very original behavior. Only return realmaware client through the new builder method.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r393863077
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -932,73 +933,13 @@ public void close() {
}
}
- public static class Builder {
- private String _zkAddress;
- private RealmAwareZkClient.RealmMode _realmMode;
- private RealmAwareZkClient.RealmAwareZkConnectionConfig _realmAwareZkConnectionConfig;
- private RealmAwareZkClient.RealmAwareZkClientConfig _realmAwareZkClientConfig;
-
+ public static class Builder extends GenericZkHelixApiBuilder<Builder> {
Review comment:
Discussed with @jiajunwang offline. We are keeping ZkHelix as is.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395807333
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkCacheBaseDataAccessor.java
##########
@@ -126,41 +117,9 @@ public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String
public ZkCacheBaseDataAccessor(String zkAddress, ZkSerializer serializer, String chrootPath,
Review comment:
This is already deprecated..
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r393881286
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java
##########
@@ -142,22 +142,22 @@ public ZkBaseDataAccessor(RealmAwareZkClient zkClient) {
}
private ZkBaseDataAccessor(Builder<T> builder) {
- switch (builder.realmMode) {
+ switch (builder.getRealmMode()) {
case MULTI_REALM:
try {
- if (builder.zkClientType == ZkClientType.DEDICATED) {
+ if (builder._zkClientType == ZkClientType.DEDICATED) {
Review comment:
So as for the logic that creates which ZkClient to use -I've actually considered an option to include it in the builder, but I think since this is a construction logic for the API, I've explicitly decided to leave it in the constructor
My philosophy is that construction logic ("business logic" for the data accessors) should be left in the constructor, and builder should only set the parameters and validate.
But I see some duplicate logic in ZkBaseDataAccessor and ZkCacheBaseDataAccessor -
So I can actually abstract that logic out into ZkBaseDataAccessor.createZkClient(), and make it a static method that can be used in both accessors. That way, 1) we still keep the construction logic in the accessors 2) Builders have a clean separation and only contain parameter setters and validation logic, and 3) we reduce code duplication.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395734207
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -384,14 +373,26 @@ protected void validate() {
+ _zkAddress + " RealmAwareZkConnectionConfig: " + _realmAwareZkConnectionConfig);
}
}
+ initializeConfigsIfNull();
+ }
- // Resolve all default values
- if (_realmAwareZkConnectionConfig == null) {
- _realmAwareZkConnectionConfig =
- new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
- if (_realmAwareZkClientConfig == null) {
- _realmAwareZkClientConfig = new RealmAwareZkClient.RealmAwareZkClientConfig();
+ /**
+ * Creates a RealmAwareZkClient for ZkHelixClusterVerifiers.
+ * Note that DedicatedZkClient is used whether it's multi-realm or single-realm.
+ * @return
+ */
+ private RealmAwareZkClient createZkClientFromBuilderForVerifier() {
Review comment:
We can do that.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395391245
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -87,26 +88,7 @@
* @param builder
*/
private ConfigAccessor(Builder builder) {
Review comment:
In order to not expose createZkClient(), I'll make that have protected access and modify the constructors.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395393774
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/api/client/ZkClientType.java
##########
@@ -0,0 +1,26 @@
+package org.apache.helix.zookeeper.api.client;
+
+/*
+ * 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 enum ZkClientType {
Review comment:
Added
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #899: Use
Java Generics and inheritance to reduce duplicate code in Helix API Builders
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #899: Use Java Generics and inheritance to reduce duplicate code in Helix API Builders
URL: https://github.com/apache/helix/pull/899#discussion_r395454217
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterVerifiers/ZkHelixClusterVerifier.java
##########
@@ -384,14 +373,26 @@ protected void validate() {
+ _zkAddress + " RealmAwareZkConnectionConfig: " + _realmAwareZkConnectionConfig);
}
}
+ initializeConfigsIfNull();
+ }
- // Resolve all default values
- if (_realmAwareZkConnectionConfig == null) {
- _realmAwareZkConnectionConfig =
- new RealmAwareZkClient.RealmAwareZkConnectionConfig.Builder().build();
- }
- if (_realmAwareZkClientConfig == null) {
- _realmAwareZkClientConfig = new RealmAwareZkClient.RealmAwareZkClientConfig();
+ /**
+ * Creates a RealmAwareZkClient for ZkHelixClusterVerifiers.
+ * Note that DedicatedZkClient is used whether it's multi-realm or single-realm.
+ * @return
+ */
+ private RealmAwareZkClient createZkClientFromBuilderForVerifier() {
Review comment:
Why not override createZkClient() 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org