fapifta commented on pull request #1405: URL: https://github.com/apache/hadoop-ozone/pull/1405#issuecomment-692291518
Hi @avijayanhwx, thank you for addressing my comments, the current solution seems to be good enough. As we discussed in a live conversation, I agree that introducing the EnumMap has consequences on how we handle the OM requests, and probably should go in an other patch, though I still believe we should work on this, and try to leave out hashing as well later. So on the main points we got to a status where I can comfortably +1 the changeset. There is one small typo which should be fixed before we are commit it. (I add an inline comment for that.) I have a few notes though on naming, and some possible extractions that can help the understandability of the code: - In TestOmVersionManagerRequestFactory.java line 75, the test method might be named to something that implies we are checking for the existence of the getRequestType method as well, or if the method does not exists we should fail the test with a custom error message. - In LayoutVersionInstanceFactory.java: getInstances() method could return an UnModifiableMap instead of a copy, and we can use default visibility we should name the long conditional expressions, in line 193, and 199 in the register method there is a large system of if statements from line 108, I think we can reorganise this to make it simpler to understand, if we return false first if that needs to be returned, then do a poll() if required, and then offer and return true otherwise. In this case we might extract and name the conditions and with that we might not need that much documentation either. None of the above I think is absolutely necessary, look at them as suggestions for your consideration. ---------------------------------------------------------------- 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 --------------------------------------------------------------------- To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
