huaxiangsun commented on PR #4608: URL: https://github.com/apache/hadoop/pull/4608#issuecomment-1198721463
> code in s3afs and the tests all look good. > > But i think the changes should be restricted to s3a fs, even if duplicates a bit of the superclass. > > That whole section in the javadocs at the top of the Filesystem. class explains why; you've already seen some of those test values but it's and external implementations which we don't have control of. Adding anything is making a commitment to preserve a new public API forever. Thanks! Points well taken. > > I will be happier if you were just do it all in S3AFileSystem. > > override deleteOnExit(Path f) and > > 1. skip the exists check because it doesn't do the right thing if you call the method while writing a file, because the file isn't visible until close. saves HEAD/List probes too > 2. save the list to a set local to s3a fs. you could make its getter protected so that mock test can access it. Got it, for s3a fs, the deleteOnExit set is going to be in s3a. > > now, what about an integration test too? Will try to add an IT. > > add a test case which creates a new fs in the test case (keeping the normal getFileSystem() fs for assertions) > > * adds a file which doesn't exist > * adds a file which doesn't exist, then create it > * adds a file which does exist > * add a directory path > * close the fs, use ContractTestUtils methods to assert the files and dirs are gone. > > seem good? Sounds good to me, thanks for the feedbacks. May come back to you for test case, going to check it out first by myself. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
