saintstack commented on code in PR #4723:
URL: https://github.com/apache/hadoop/pull/4723#discussion_r949291014
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/qjournal/server/JournalNodeSyncer.java:
##########
@@ -153,7 +156,8 @@ private boolean getOtherJournalNodeProxies() {
LOG.warn("Could not add proxy for Journal at addresss " + addr, e);
}
}
- if (otherJNProxies.isEmpty()) {
+ // Check if any of there are any resolvable JournalNodes before starting
the sync.
Review Comment:
s/there/them/? Extra 'any'?
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/qjournal/server/JournalNodeSyncer.java:
##########
@@ -310,12 +314,23 @@ private List<InetSocketAddress>
getOtherJournalNodeAddrs() {
return null;
}
- private List<InetSocketAddress> getJournalAddrList(String uriStr) throws
+ @VisibleForTesting
+ protected List<InetSocketAddress> getJournalAddrList(String uriStr) throws
Review Comment:
This is @private audience class. We are changing method visibility here just
for testing. I think that ok.
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/qjournal/server/JournalNodeSyncer.java:
##########
@@ -310,12 +314,23 @@ private List<InetSocketAddress>
getOtherJournalNodeAddrs() {
return null;
}
- private List<InetSocketAddress> getJournalAddrList(String uriStr) throws
+ @VisibleForTesting
+ protected List<InetSocketAddress> getJournalAddrList(String uriStr) throws
URISyntaxException,
IOException {
URI uri = new URI(uriStr);
- return Util.getLoggerAddresses(uri,
- new HashSet<>(Arrays.asList(jn.getBoundIpcAddress())), conf);
+
+ InetSocketAddress boundIpcAddress = jn.getBoundIpcAddress();
+ Set<InetSocketAddress> excluded = Sets.newHashSet(boundIpcAddress);
+ List<InetSocketAddress> addrList = Util.getLoggerAddresses(uri, excluded,
conf);
+
+ // Exclude the current JournalNode instance. If we are bound to a local
address on the same
Review Comment:
exclude 'any' port?
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/qjournal/server/TestJournalNodeSync.java:
##########
@@ -420,7 +456,7 @@ public void testSyncDuringRollingUpgrade() throws Exception
{
// Restart the current standby NN (previously active)
dfsCluster.restartNameNode(standbyNNindex, true,
"-rollingUpgrade", "started");
- Assert.assertEquals(info, dfsActive.rollingUpgrade(
+ assertEquals(info, dfsActive.rollingUpgrade(
Review Comment:
FYI, best to not pollute your PR w/ this sort of non-related clean-up.
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/qjournal/server/TestJournalNodeSync.java:
##########
@@ -96,12 +99,45 @@ public void shutDownMiniCluster() throws IOException {
}
}
+ /**
+ * Test that the "self exclusion" works when there are multiple JournalNode
instances running on
+ * the same server, but on different ports.
+ */
+ @Test
+ public void testJournalNodeExcludesSelfMultilpePorts() throws
URISyntaxException, IOException {
+ String uri =
qjmhaCluster.getJournalCluster().getQuorumJournalURI("ns1").toString();
+ JournalNodeSyncer syncer =
jCluster.getJournalNode(0).getJournalSyncer("ns1");
+
+ // Test: Get the Journal address list for the default configuration
+ List<InetSocketAddress> addrList = syncer.getJournalAddrList(uri);
+
+ // Verify: One of the addresses should be excluded so that the node isn't
syncing with itself
+ assertEquals(2, addrList.size());
+ }
+
+ /**
+ * Test that the "self exclusion" works when there a host uses a wildcard
address.
+ */
+ @Test
+ public void testJournalNodeExcludesSelfWildCard() throws URISyntaxException,
IOException {
+ String uri =
qjmhaCluster.getJournalCluster().getQuorumJournalURI("ns1").toString();
+ JournalNodeSyncer syncer =
jCluster.getJournalNode(0).getJournalSyncer("ns1");
+
+ // Test: Request the same Journal address list, but using the IPv4
"0.0.0.0" which is commonly
+ // used as a bind host.
+ String boundHostUri = uri.replaceAll("127.0.0.1", "0.0.0.0");
+ List<InetSocketAddress> boundHostAddrList =
syncer.getJournalAddrList(boundHostUri);
+
+ // Verify: One of the address should be excluded so that the node isn't
syncing with itself
+ assertEquals(2, boundHostAddrList.size());
+ }
+
Review Comment:
Nice tests.
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/qjournal/server/JournalNodeSyncer.java:
##########
@@ -153,7 +156,8 @@ private boolean getOtherJournalNodeProxies() {
LOG.warn("Could not add proxy for Journal at addresss " + addr, e);
}
}
- if (otherJNProxies.isEmpty()) {
+ // Check if any of there are any resolvable JournalNodes before starting
the sync.
+ if (otherJNProxies.stream().filter(jnp ->
!jnp.jnAddr.isUnresolved()).count() == 0) {
Review Comment:
We are waiting on them to resolve? If resolve fails, we make no progress ...
This is better (and aligns w/ other checks done above in this method returning
false...)
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/qjournal/server/JournalNodeSyncer.java:
##########
@@ -310,12 +314,23 @@ private List<InetSocketAddress>
getOtherJournalNodeAddrs() {
return null;
}
- private List<InetSocketAddress> getJournalAddrList(String uriStr) throws
+ @VisibleForTesting
+ protected List<InetSocketAddress> getJournalAddrList(String uriStr) throws
URISyntaxException,
IOException {
URI uri = new URI(uriStr);
- return Util.getLoggerAddresses(uri,
- new HashSet<>(Arrays.asList(jn.getBoundIpcAddress())), conf);
+
+ InetSocketAddress boundIpcAddress = jn.getBoundIpcAddress();
+ Set<InetSocketAddress> excluded = Sets.newHashSet(boundIpcAddress);
+ List<InetSocketAddress> addrList = Util.getLoggerAddresses(uri, excluded,
conf);
+
+ // Exclude the current JournalNode instance. If we are bound to a local
address on the same
Review Comment:
Does this comment read right to you? Same "host"?
--
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]
