fapifta commented on pull request #1425: URL: https://github.com/apache/hadoop-ozone/pull/1425#issuecomment-693386962
Hi @llemec, thank you for working on this patch, I think overall the direction is good here, though I miss a few things, and I have a stylistic problem with the proposed changes. In test testgetFromProtobufOneMetadataOneAcl(), we do check only the prefix path, metadata size and value, and acl size, should we also check for the acl value here? I think we should, if not, can you give some reasons why not? Also, can you pleae remove the two empty lines at the end of this test method? In test testGetProtobuf() we are also checking just for the count of object, should we check for the contents as well? What I miss is some corner cases, as it seems, the code is prepared for metadata being null, and acl list being null, but the tests does not cover that scenario, so probably we should do so as well. The stylistic problem I have is minor, and probably it is the matter of taste, I don't see the utility in having the TestInstanceHelper class, as at this point, that contains methods only for this test. In case these can be and should be used elsewhere we probably should move it out to a utility class at that time, if we notice it. If we miss it next time, I am more on the side of having duplications in the test code in well named methods, than having utility classes that might not get used ever elsewhere or having a too general name and purpose, while in the meantime incur some maintenance and cognitive costs whenever someone looks at them, so I prefer to put these kind of methods as private methods in my test class. If I convinced you, you can move it, or leave it as it is, up to you, I can accept this approach as well. ---------------------------------------------------------------- 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
