You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2022/08/18 15:36:02 UTC
[GitHub] [hadoop] saintstack commented on a diff in pull request #4723: HDFS-16684. Exclude the current JournalNode
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org