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
