fapifta commented on issue #1596: HDDS-2233 - Remove ByteStringHelper and refactor the code to the place where it used URL: https://github.com/apache/hadoop/pull/1596#issuecomment-538868639 Hi @bshashikant first of all, thank you for the review. Let me share my thought process when I selected to have a utility method that is dependent on the Configuration we have: - the logic of conversion is changing based on a configuration value - we don't use the utility method so far in any other context where the decision of which implementation is chosen is made based on anything else then configuration - this way I need to write the reading and handling of the related Configuration value once - this coupling of the implementation choosing logic and configuration I think is best to be encapsulated into the utility method that creates our conversion strategy, to express it clearly that this is dependent on a configuration value in the current environment. So all in all I would refuse to have the method with a boolean parameter for now as I don't see its benefits at the moment, and also because if there will ever be an other arbitrary decision making process coexisting besides the using of the Configuration, then I think we can factor a method out from the current one, which will accept a boolean as a parameter, or based on other parameters we can implement the decision logic for other type of clients when there is a need. Does this sound reasonable and acceptable?
---------------------------------------------------------------- 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]
