[
https://issues.apache.org/jira/browse/HDFS-16684?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17581400#comment-17581400
]
ASF GitHub Bot commented on HDFS-16684:
---------------------------------------
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"?
> Exclude self from JournalNodeSyncer when using a bind host
> ----------------------------------------------------------
>
> Key: HDFS-16684
> URL: https://issues.apache.org/jira/browse/HDFS-16684
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: journal-node
> Environment: Running with Java 11 and bind addresses set to 0.0.0.0.
> Reporter: Steve Vaughan
> Assignee: Steve Vaughan
> Priority: Major
> Labels: pull-request-available
>
> The JournalNodeSyncer will include the local instance in syncing when using a
> bind host (e.g. 0.0.0.0). There is a mechanism that is supposed to exclude
> the local instance, but it doesn't recognize the meta-address as a local
> address.
> Running with bind addresses set to 0.0.0.0, the JournalNodeSyncer will log
> attempts to sync with itself as part of the normal syncing rotation. For an
> HA configuration running 3 JournalNodes, the "other" list used by the
> JournalNodeSyncer will include 3 proxies.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
