pan3793 commented on code in PR #8717:
URL: https://github.com/apache/hadoop/pull/8717#discussion_r3946403709
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestStandbyCheckpoints.java:
##########
@@ -764,13 +764,29 @@ public void testLastCheckpointTime() throws Exception {
nns[0].getRpcServer().rollEditLog();
HATestUtil.waitForCheckpoint(cluster, 0, ImmutableList.of(23));
+ // The wait above only says the active holds the new image. Every standby
+ // builds its own checkpoint and any of them may be the one that uploaded
+ // it, and the one that did stamps its own lastCheckpointTime only once
+ // doCheckpoint() has returned. So nns[1] can still be reporting the
+ // previous checkpoint here, which reads as an interval of zero. Wait for
+ // its time to move before taking the pair. The active stamps its own
+ // while it receives the upload, inside that same doCheckpoint(), so by
+ // then it has necessarily moved too.
+ GenericTestUtils.waitFor(
Review Comment:
`StandbyCheckpointer.doWork` stamps `lastCheckpointTime` unconditionally
after `doCheckpoint()`, including the "Skipping" branch when no new txns
arrived, so this predicate also passes on a period tick that made no
checkpoint. `HATestUtil.waitForCheckpoint(cluster, 1, ImmutableList.of(23))`
before it anchors the wait to the real one.
Nit: the 4-arg `GenericTestUtils.waitFor` overload takes an error message;
on timeout this one shows only "Timed out waiting for condition" without
`snnCheckpointTime1` or the current value.
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestStandbyCheckpoints.java:
##########
@@ -764,13 +764,29 @@ public void testLastCheckpointTime() throws Exception {
nns[0].getRpcServer().rollEditLog();
HATestUtil.waitForCheckpoint(cluster, 0, ImmutableList.of(23));
+ // The wait above only says the active holds the new image. Every standby
Review Comment:
`nns[2]` is an observer by L761, and observers run no `StandbyCheckpointer`
(`FSNamesystem.startStandbyServices` creates one only when `!isObserver`), so
`nns[1]` is the only possible uploader here. Suggest rewording: the gap is
between `ImageServlet` stamping the active during the upload and `nns[1]`
stamping itself after `doCheckpoint()` returns.
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/http/TestSSLHttpServerMTLS.java:
##########
@@ -145,6 +148,15 @@ public void testUntrustedClientIsRejected() throws
Exception {
HttpsURLConnection conn = (HttpsURLConnection) url.openConnection();
// presents untrustedCert; server cert is trusted via no-op TrustManager
KeyStoreTestUtil.setAllowAllSSL(conn, untrustedCert, untrustedKeyPair);
- assertThrows(SSLHandshakeException.class, () -> conn.getInputStream());
+ // The server rejects the certificate as soon as it arrives and drops the
+ // connection, which races the client's own last handshake flight. When
+ // the close wins the client fails writing that flight and never reads
+ // the alert, so the refusal reaches it as a SocketException rather than
+ // an SSLHandshakeException. What the server guarantees is that the
+ // request is refused, not which of the two the client gets to see.
+ IOException e =
+ assertThrows(IOException.class, () -> conn.getInputStream());
+ assertTrue(e instanceof SSLException || e instanceof SocketException,
Review Comment:
Verified locally on JDK 17 with @RepeatedTest(50). The server negotiates
TLSv1.2 (hadoop.ssl.enabled.protocols default), and the new assertion passes
50/50 there. With hadoop.ssl.enabled.protocols=TLSv1.3 it fails 33/50: the
handshake completes client-side before the server verifies the cert, the HTTP
request write fails, and HttpURLConnection throws a bare IOException("Error
writing to server") with no cause (HttpURLConnection.java:776), which is
neither SSLException nor SocketException.
Suggest asserting IOException and excluding only ConnectException; that
passes 50/50 under both protocols here.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/logaggregation/TestLogAggregationService.java:
##########
@@ -264,7 +264,12 @@ private void verifyLocalFileDeletion(
GenericTestUtils.waitFor(() -> !f.exists(), 1000, 1000 * 50);
assertFalse(f.exists(), "File [" + f + "] was not deleted");
}
- assertFalse(app1LogDir.exists(), "Directory [" + app1LogDir + "] was not
deleted");
+ // DeletionService removes the files before the directories that hold
Review Comment:
The container-file and app-dir deletes are two independent recursive
`FileDeletionTask`s scheduled with zero delay on the `DeletionService` pool,
with no dependency between them, so "removes the files before the directories"
is not the mechanism; the app-dir task can run first or concurrently. A single
wait on `app1LogDir` subsumes the four per-file waits.
Nit: the 4-arg `GenericTestUtils.waitFor` overload carries the message; the
`assertFalse` after the wait is unreachable on the failure path, so "Directory
[...] was not deleted" never shows.
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestStandbyCheckpoints.java:
##########
@@ -764,13 +764,29 @@ public void testLastCheckpointTime() throws Exception {
nns[0].getRpcServer().rollEditLog();
HATestUtil.waitForCheckpoint(cluster, 0, ImmutableList.of(23));
+ // The wait above only says the active holds the new image. Every standby
+ // builds its own checkpoint and any of them may be the one that uploaded
+ // it, and the one that did stamps its own lastCheckpointTime only once
+ // doCheckpoint() has returned. So nns[1] can still be reporting the
+ // previous checkpoint here, which reads as an interval of zero. Wait for
+ // its time to move before taking the pair. The active stamps its own
+ // while it receives the upload, inside that same doCheckpoint(), so by
+ // then it has necessarily moved too.
+ GenericTestUtils.waitFor(
+ () -> nns[1].getNamesystem().getStandbyLastCheckpointTime()
+ > snnCheckpointTime1, 100, 30000);
+
long snnCheckpointTime2 =
nns[1].getNamesystem().getStandbyLastCheckpointTime();
long annCheckpointTime2 = nns[0].getNamesystem().getLastCheckpointTime();
Review Comment:
The active stamps `mostRecentCheckpointTime` (`FSImage.java:1486`) after the
rename that `waitForCheckpoint` observes (`:1481`), and the standby predicate
above can already be true from an earlier stamp, so "has necessarily moved too"
is not guaranteed. A symmetric wait on
`nns[0].getNamesystem().getLastCheckpointTime() > annCheckpointTime1` closes
it. Microsecond window, so a nit.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice-hbase-tests/src/test/java/org/apache/hadoop/yarn/server/timelineservice/storage/TestTimelineReaderHBaseDown.java:
##########
@@ -67,6 +68,7 @@ public void testTimelineReaderHBaseUp() throws Exception {
throw e;
}
} finally {
+ stopServer(server);
Review Comment:
`stopServer(server)` runs unguarded as the first statement of this `finally`
(same at L127, L164, L211). `AbstractService.stop()` rethrows any `serviceStop`
failure, and `TimelineStorageMonitor.stop()` throws `InterruptedException` from
`awaitTermination` if a `@Timeout` interrupt lands there, so a throwing stop
skips `util.shutdownMiniCluster()` and replaces the test's own exception. Base
code reached `shutdownMiniCluster()` unconditionally.
`ServiceOperations.stopQuietly(server)` is the existing null-safe helper for
this. With it the `= null` locals and the `stopServer` helper can go; stopping
a never-inited service is a legal no-op.
--
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]
