HIVE-27317: Temporary (local) session files cleanup improvements #4403
Conversation
| // Note: Give scratch dirs all the write permissions | ||
| FsPermission allPermissions = new FsPermission((short)00777); | ||
| Path scratchDir = new Path(HiveConf.getVar(conf, HiveConf.ConfVars.SCRATCHDIR)); | ||
| Utilities.createDirsWithPermission(conf, scratchDir, allPermissions, true); |
There was a problem hiding this comment.
hdfs scratch dir is all writable.
There was a problem hiding this comment.
could we please not use the default scratch dir in a test? create some temp one
There was a problem hiding this comment.
@deniskuzZ good idea to avoid all the possible conflicts. The test has its own HiveConf, so I have set scratchdir properties as below;
String tmpDir = System.getProperty("test.tmp.dir");
conf.set("hive.exec.scratchdir", tmpDir + "/hive-27317-hdfsscratchdir");
conf.set("hive.exec.local.scratchdir", tmpDir + "/hive-27317-localscratchdir");
@veghlaci05 about deleting the files/dirs; as the both tmp dirs are specific to the test now, I have deleted the both of them after the test;
fs.delete(localTmpDir, true);
fs.delete(scratchDir, true);
The result is as expected, it creates specified tmp dirs;
> ll target/tmp/
drwxrwxrwx 3 sercan sercan 4096 Jun 12 09:22 hive-27317-hdfsscratchdir/
drwxrwxrwx 3 sercan sercan 4096 Jun 12 09:22 hive-27317-localscratchdir/
and deletes both of them after the test.
| + appId + "-started-with-session-name-but-fail-delete.pipeout"); | ||
|
|
||
| // Create dirs/files | ||
| Utilities.createDirsWithPermission(conf, localSessionDir, allPermissions, true); |
There was a problem hiding this comment.
Local scratchdir is all writable.
|
Considering the previous issue, you should also add a teardown method to the test and delete all the created files and folders including the file with |
sercanCyberVision
force-pushed
the
HIVE-27317
branch
from
1c6c77e
to
56cf0e2
Compare
| + "' does not exist, should have not been removed!", fs.exists(localPipeOutFileFailRemove)); | ||
|
|
||
| // Clean-up | ||
| fs.delete(localTmpDir, true); |
There was a problem hiding this comment.
should it be called within finally or in @after method?
There was a problem hiding this comment.
@deniskuzZ, I have declared the custom scratchdirs out of the test, and delete them in @AfterClass which is already existed.
Executed the whole test class, looks good;
[INFO] Running org.apache.hadoop.hive.ql.session.TestClearDanglingScratchDir
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 18.943 s - in org.apache.hadoop.hive.ql.session.TestClearDanglingScratchDir
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0
56cf0e2
to
a01ed73
Compare
kgyrtkirk
added
tests failed
tests pending
and removed
tests pending
tests failed
labels
sercanCyberVision
force-pushed
the
HIVE-27317
branch
from
a01ed73
to
bc9bbdc
Compare
kgyrtkirk
added
tests unstable
tests pending
and removed
tests pending
tests unstable
labels
sercanCyberVision
force-pushed
the
HIVE-27317
branch
from
bc9bbdc
to
713fc7e
Compare
kgyrtkirk
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
sercanCyberVision
force-pushed
the
HIVE-27317
branch
from
713fc7e
to
d78c749
Compare
kgyrtkirk
added
tests pending
tests failed
and removed
tests unstable
tests pending
labels
sercanCyberVision
force-pushed
the
HIVE-27317
branch
from
d78c749
to
ee39cd7
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
…viewed by Denys Kuzmenko, Laszlo Vegh) Closes apache#4403
…viewed by Denys Kuzmenko, Laszlo Vegh) Closes apache#4403
Unit test in the initial solution caused regression to the successors unit tests as it creates scratch dirs without all writable permission. So the solution #4293 has been reverted here #4399.
What changes were proposed in this pull request?
Along with original solution, the unit test has been modified. Now, both scratch dirs have all writable permissions.
How was this patch tested?
Unit test.
@deniskuzZ Could you please review?