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: us...@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org For additional commands, e-mail: reviews-h...@helix.apache.org
