jiajunwang commented on issue #682: Implement sharding key trie node class URL: https://github.com/apache/helix/pull/682#issuecomment-579469196 > Replying to both comments from @jiajunwang and @narendly here, as both comments revolve around `getZkRealmAddress()`. > I agree that `getZkRealmAddress()` should not be used for non-leaf nodes. As of right now, I have added the exception throwing behavior per @narendly 's suggestion, however, at this moment I do not agree with this approach. Throwing an exception implies that `getZkRealmAddress()` should not be called under certain conditions, and that `zkRealmAddress` should not exist under these conditions; it is a better practice to ensure that such a function is unavailable in the first place (under these conditions), and that the object is always in a valid state. > I believe @jiajunwang 's proposed design is one of the only ways to achieve this, but it'll be a complete redo on the current design. Per @narendly 's suggestions, I purpose to move on with the current design for now, and revisit this class as more usage to this class is consolidated. With more usage, we will have a better context on how the class should behave, which can help making a decision. This class has been stalling the progress on other components for longer than planned, which we should avoid. @NealSun96 Let's do the right thing. Schedule is something we need to concern. But if you leave this kind of known potential issues at this early stage, it tends to cost you much more time later to fix later.
---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
