abstractdog commented on code in PR #520:
URL: https://github.com/apache/tez/pull/520#discussion_r3673333787
##########
tez-tests/src/test/java/org/apache/tez/test/TestAMRecoveryAggregationBroadcast.java:
##########
@@ -339,12 +346,18 @@ private DAG createDAG(String dagName) throws Exception {
return dag;
}
- TezCounters runDAGAndVerify(DAG dag, boolean killAM) throws Exception {
+ TezCounters runDAGAndVerify(DAG dag, boolean killAM, List<String>
vertexNamesToWaitFor) throws Exception {
tezSession.waitTillReady();
DAGClient dagClient = tezSession.submitDAG(dag);
if (killAM) {
- TimeUnit.SECONDS.sleep(10);
+ // Deterministic wait: block until every named upstream vertex reaches
+ // SUCCEEDED. Replaces a fixed Thread.sleep(10s) which was too short on
+ // slow CI machines and caused the recovery-log assertions below to
+ // fail intermittently.
Review Comment:
only useful information for the long term here is: "Deterministic wait:
block until every named upstream vertex reaches SUCCEEDED"
##########
tez-api/src/test/java/org/apache/tez/client/TestTezClientUtils.java:
##########
@@ -1003,4 +1008,41 @@ public void
testSessionCredentialsMergedBeforeAmConfigCredentials() throws Excep
// session token should be applied while creating ContainerLaunchContext
assertEquals(sessionToken, amLaunchCredentials.getToken(tokenType));
}
+
+ /**
+ * Covers the YARN-808 guard in TezClientUtils#getAMProxy
+ */
+ @Test
+ @Timeout(value = 5000, unit = TimeUnit.MILLISECONDS)
+ public void testGetAMProxyReturnsNullWhenRpcEndpointNotAvailable() throws
Exception {
+ TezConfiguration conf = new TezConfiguration();
+ ApplicationId appId = ApplicationId.newInstance(1L, 1);
+ UserGroupInformation ugi = UserGroupInformation.getCurrentUser();
+
+ // Case 1: rpcPort == -1 (YARN-808 gap — AM container up, RPC not bound)
+ assertNull(TezClientUtils.getAMProxy(
+ newRunningFrameworkClient(appId, "somehost", -1), conf, appId, ugi),
+ "rpcPort == -1 should return null");
+
+ // Case 2: rpcPort == 0 (protobuf wire default)
+ assertNull(TezClientUtils.getAMProxy(
+ newRunningFrameworkClient(appId, "somehost", 0), conf, appId, ugi),
+ "rpcPort == 0 should return null");
+
+ // Case 3: host == "N/A" (RM has not yet received AM registration)
+ assertNull(TezClientUtils.getAMProxy(
+ newRunningFrameworkClient(appId, "N/A", 8080), conf, appId, ugi),
+ "host == N/A should return null");
Review Comment:
`host == null` case is missing here
##########
tez-tests/src/test/java/org/apache/tez/test/TestAMRecoveryAggregationBroadcast.java:
##########
@@ -119,6 +120,10 @@ public class TestAMRecoveryAggregationBroadcast {
private TezConfiguration tezConf;
private TezClient tezSession;
+ // Per-test unique output path.
+ // replaces the former static OUT_PATH so that a stale file from a prior run
in the same forked JVM
+ // cannot bleed into a later run.
Review Comment:
this comment is not needed: code reader is not necessarily interested in the
history of how this field came
--
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]
