abstractdog commented on code in PR #523:
URL: https://github.com/apache/tez/pull/523#discussion_r3595625141
##########
tez-api/src/test/java/org/apache/tez/client/TestTezClient.java:
##########
@@ -300,6 +293,54 @@ private void _testTezClientSessionLargeDAGPlan(int
maxIPCMsgSize, int payloadSiz
}
}
+ @Test
+ @Timeout(value = 10000, unit = TimeUnit.MILLISECONDS)
+ public void testSessionLargeDAGPlanWithLeftoverPlanFile() throws Exception {
+ // Simulates long-running/external sessions (e.g. Hive with external
session pools): the
+ // application and its staging dir outlive the TezClient instances, which
are recreated per
+ // query with their serialized-plan-file counter restarting from 0. A plan
file left behind
+ // by a previous client generation must not fail subsequent submissions
with
+ // FileAlreadyExistsException.
+ TezConfiguration conf = new TezConfiguration();
+ conf.setInt(CommonConfigurationKeys.IPC_MAXIMUM_DATA_LENGTH, 1024 * 1024);
+ conf.set(TezConfiguration.TEZ_AM_STAGING_DIR,
STAGING_DIR.getAbsolutePath());
+
+ Path baseStagingPath = TezCommonUtils.getTezBaseStagingPath(conf);
+ FileSystem localFs = FileSystem.getLocal(conf);
+ localFs.delete(baseStagingPath, true);
+
+ // first client generation writes tez-dag.pb1 into the session's staging
dir and is then
+ // abandoned without stop() - the session and its staging dir keep running
+ TezClientForTest client1 = configureAndCreateTezClient(null, true, conf);
+ client1.start();
+ submitDAGAndCaptureRequest(client1, largeDAG("DAG-gen1", 2 * 1024 * 1024));
+
Review Comment:
make an assertion here too about serialized path:
```
assertTrue(request.hasSerializedRequestPath());
```
##########
tez-api/src/test/java/org/apache/tez/client/TestTezClient.java:
##########
@@ -300,6 +293,54 @@ private void _testTezClientSessionLargeDAGPlan(int
maxIPCMsgSize, int payloadSiz
}
}
+ @Test
+ @Timeout(value = 10000, unit = TimeUnit.MILLISECONDS)
+ public void testSessionLargeDAGPlanWithLeftoverPlanFile() throws Exception {
+ // Simulates long-running/external sessions (e.g. Hive with external
session pools): the
+ // application and its staging dir outlive the TezClient instances, which
are recreated per
+ // query with their serialized-plan-file counter restarting from 0. A plan
file left behind
+ // by a previous client generation must not fail subsequent submissions
with
+ // FileAlreadyExistsException.
+ TezConfiguration conf = new TezConfiguration();
+ conf.setInt(CommonConfigurationKeys.IPC_MAXIMUM_DATA_LENGTH, 1024 * 1024);
+ conf.set(TezConfiguration.TEZ_AM_STAGING_DIR,
STAGING_DIR.getAbsolutePath());
+
+ Path baseStagingPath = TezCommonUtils.getTezBaseStagingPath(conf);
+ FileSystem localFs = FileSystem.getLocal(conf);
+ localFs.delete(baseStagingPath, true);
+
+ // first client generation writes tez-dag.pb1 into the session's staging
dir and is then
+ // abandoned without stop() - the session and its staging dir keep running
+ TezClientForTest client1 = configureAndCreateTezClient(null, true, conf);
+ client1.start();
+ submitDAGAndCaptureRequest(client1, largeDAG("DAG-gen1", 2 * 1024 * 1024));
+
+ // second client generation reconnects to the same session: same appId,
same staging dir,
+ // restarted plan-file counter - it computes the same tez-dag.pb1 path
+ TezClientForTest client2 = configureAndCreateTezClient(null, true, conf);
+ client2.start();
+ SubmitDAGRequestProto request = submitDAGAndCaptureRequest(client2,
largeDAG("DAG-gen2", 2 * 1024 * 1024));
+ client2.stop();
+
+ assertTrue(request.hasSerializedRequestPath());
Review Comment:
this assertion can go right after the `SubmitDAGRequestProto request =
submitDAGAndCaptureRequest` to emphasize that `client2.stop()` doesn't have an
effect on it (as far as I can understand)
##########
tez-api/src/test/java/org/apache/tez/client/TestTezClient.java:
##########
@@ -300,6 +293,54 @@ private void _testTezClientSessionLargeDAGPlan(int
maxIPCMsgSize, int payloadSiz
}
}
+ @Test
+ @Timeout(value = 10000, unit = TimeUnit.MILLISECONDS)
+ public void testSessionLargeDAGPlanWithLeftoverPlanFile() throws Exception {
+ // Simulates long-running/external sessions (e.g. Hive with external
session pools): the
+ // application and its staging dir outlive the TezClient instances, which
are recreated per
+ // query with their serialized-plan-file counter restarting from 0. A plan
file left behind
+ // by a previous client generation must not fail subsequent submissions
with
+ // FileAlreadyExistsException.
+ TezConfiguration conf = new TezConfiguration();
+ conf.setInt(CommonConfigurationKeys.IPC_MAXIMUM_DATA_LENGTH, 1024 * 1024);
+ conf.set(TezConfiguration.TEZ_AM_STAGING_DIR,
STAGING_DIR.getAbsolutePath());
+
+ Path baseStagingPath = TezCommonUtils.getTezBaseStagingPath(conf);
+ FileSystem localFs = FileSystem.getLocal(conf);
+ localFs.delete(baseStagingPath, true);
+
+ // first client generation writes tez-dag.pb1 into the session's staging
dir and is then
Review Comment:
update "tez-dag.pb1" if you consider my other comment about the modified
filename
##########
tez-api/src/test/java/org/apache/tez/client/TestTezClient.java:
##########
@@ -300,6 +293,54 @@ private void _testTezClientSessionLargeDAGPlan(int
maxIPCMsgSize, int payloadSiz
}
}
+ @Test
+ @Timeout(value = 10000, unit = TimeUnit.MILLISECONDS)
+ public void testSessionLargeDAGPlanWithLeftoverPlanFile() throws Exception {
Review Comment:
it would be nice to make multiple assertions in this unit test method
whether the dag plan file exists or not
currently, name suggests `LeftoverPlanFile`, but file existence is not
checked; it would be nice to see that, especially as
`DAGClientAMProtocolBlockingPBServerImpl` has been changed to delete after
consuming, which makes me think there is no leftover at all
##########
tez-api/src/main/java/org/apache/tez/client/TezClient.java:
##########
@@ -692,7 +692,9 @@ private DAGClient submitDAGSession(DAG dag) throws
TezException, IOException {
serializedSubmitDAGPlanRequestCounter.incrementAndGet());
Review Comment:
as we're already changing this codepath, can we make the path look a bit
better? I don't like that the counter is appended after the file extension,
like :)
```
application_1783445457232_0389/tez-dag.pb1
application_1783445457232_0389/tez-dag.pb2
```
instead it could be e.g.:
```
application_1783445457232_0389/tez-dag-1.pb
application_1783445457232_0389/tez-dag-2.pb
```
##########
tez-api/src/main/java/org/apache/tez/client/TezClient.java:
##########
@@ -692,7 +692,9 @@ private DAGClient submitDAGSession(DAG dag) throws
TezException, IOException {
serializedSubmitDAGPlanRequestCounter.incrementAndGet());
FileSystem fs = dagPlanPath.getFileSystem(stagingFs.getConf());
- try (FSDataOutputStream fsDataOutputStream = fs.create(dagPlanPath,
false)) {
+ // Overwrite a possible leftover plan file: when the session outlives
the TezClient instances,
+ // a file with the same name may have been left behind by an already
consumed submission.
Review Comment:
"left behind by an already consumed submission." is this true?
with the new finally block introduced in the DAG client-server, we can make
sure that if it's consumed, it's deleted, right?
btw, I don't mind if we take care of this in two places (like: overwrite +
delete in finally), it just doesn't seem 100% accurate to say that a consumed
file could be left behind
##########
tez-api/src/test/java/org/apache/tez/client/TestTezClient.java:
##########
@@ -300,6 +293,54 @@ private void _testTezClientSessionLargeDAGPlan(int
maxIPCMsgSize, int payloadSiz
}
}
+ @Test
+ @Timeout(value = 10000, unit = TimeUnit.MILLISECONDS)
+ public void testSessionLargeDAGPlanWithLeftoverPlanFile() throws Exception {
+ // Simulates long-running/external sessions (e.g. Hive with external
session pools): the
+ // application and its staging dir outlive the TezClient instances, which
are recreated per
+ // query with their serialized-plan-file counter restarting from 0. A plan
file left behind
+ // by a previous client generation must not fail subsequent submissions
with
+ // FileAlreadyExistsException.
+ TezConfiguration conf = new TezConfiguration();
+ conf.setInt(CommonConfigurationKeys.IPC_MAXIMUM_DATA_LENGTH, 1024 * 1024);
+ conf.set(TezConfiguration.TEZ_AM_STAGING_DIR,
STAGING_DIR.getAbsolutePath());
+
+ Path baseStagingPath = TezCommonUtils.getTezBaseStagingPath(conf);
+ FileSystem localFs = FileSystem.getLocal(conf);
+ localFs.delete(baseStagingPath, true);
+
+ // first client generation writes tez-dag.pb1 into the session's staging
dir and is then
+ // abandoned without stop() - the session and its staging dir keep running
+ TezClientForTest client1 = configureAndCreateTezClient(null, true, conf);
+ client1.start();
+ submitDAGAndCaptureRequest(client1, largeDAG("DAG-gen1", 2 * 1024 * 1024));
+
+ // second client generation reconnects to the same session: same appId,
same staging dir,
+ // restarted plan-file counter - it computes the same tez-dag.pb1 path
Review Comment:
update "tez-dag.pb1" if you consider my other comment about the modified
filename
--
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]
