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 "steveloughran (via GitHub)" <gi...@apache.org> on 2023/02/28 14:32:21 UTC
[GitHub] [hadoop] steveloughran commented on a diff in pull request #5434: HDFS-16934. org.apache.hadoop.hdfs.tools.TestDFSAdmin#testAllDatanodesReconfig regression
steveloughran commented on code in PR #5434:
URL: https://github.com/apache/hadoop/pull/5434#discussion_r1120154903
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSAdmin.java:
##########
@@ -1238,36 +1240,44 @@ public void testAllDatanodesReconfig()
when(reconfigurationUtil.parseChangedProperties(any(Configuration.class),
any(Configuration.class))).thenReturn(changes);
- assertEquals(0, admin.startReconfiguration("datanode", "livenodes"));
+ int result = admin.startReconfiguration("datanode", "livenodes");
+ Assertions.assertThat(result).isEqualTo(0);
final List<String> outsForStartReconf = new ArrayList<>();
final List<String> errsForStartReconf = new ArrayList<>();
reconfigurationOutErrFormatter("startReconfiguration", "datanode",
"livenodes", outsForStartReconf, errsForStartReconf);
- assertEquals(3, outsForStartReconf.size());
- assertEquals(0, errsForStartReconf.size());
- assertTrue(outsForStartReconf.get(0).startsWith("Started reconfiguration task on node"));
- assertTrue(outsForStartReconf.get(1).startsWith("Started reconfiguration task on node"));
- assertEquals("Starting of reconfiguration task successful on 2 nodes, failed on 0 nodes.",
- outsForStartReconf.get(2));
+ String started = "Started reconfiguration task on node";
+ String starting =
+ "Starting of reconfiguration task successful on 2 nodes, failed on 0 nodes.";
+ Assertions.assertThat(outsForStartReconf.size()).isEqualTo(3);
+ Assertions.assertThat(errsForStartReconf.size()).isEqualTo(0);
+ Assertions.assertThat(outsForStartReconf.get(0)).startsWith(started);
+ Assertions.assertThat(outsForStartReconf.get(1)).startsWith(started);
+ Assertions.assertThat(outsForStartReconf.get(2)).startsWith(starting);
Thread.sleep(1000);
final List<String> outs = new ArrayList<>();
final List<String> errs = new ArrayList<>();
awaitReconfigurationFinished("datanode", "livenodes", outs, errs);
- assertEquals(9, outs.size());
- assertEquals(0, errs.size());
+ Assertions.assertThat(outs.size()).isEqualTo(9);
+ Assertions.assertThat(errs.size()).isEqualTo(0);
LOG.info("dfsadmin -status -livenodes output:");
outs.forEach(s -> LOG.info("{}", s));
- assertTrue(outs.get(0).startsWith("Reconfiguring status for node"));
- assertTrue("SUCCESS: Changed property dfs.datanode.peer.stats.enabled".equals(outs.get(2))
- || "SUCCESS: Changed property dfs.datanode.peer.stats.enabled".equals(outs.get(1)));
- assertTrue("\tFrom: \"false\"".equals(outs.get(3)) || "\tFrom: \"false\"".equals(outs.get(2)));
- assertTrue("\tTo: \"true\"".equals(outs.get(4)) || "\tTo: \"true\"".equals(outs.get(3)));
- assertEquals("SUCCESS: Changed property dfs.datanode.peer.stats.enabled", outs.get(5));
- assertEquals("\tFrom: \"false\"", outs.get(6));
- assertEquals("\tTo: \"true\"", outs.get(7));
- assertEquals("Retrieval of reconfiguration status successful on 2 nodes, failed on 0 nodes.",
- outs.get(8));
+ Assertions.assertThat(outs.get(0)).startsWith("Reconfiguring status for node");
+
+ String success = "SUCCESS: Changed property dfs.datanode.peer.stats.enabled";
+ String from = "\tFrom: \"false\"";
+ String to = "\tTo: \"true\"";
+ String retrieval =
+ "Retrieval of reconfiguration status successful on 2 nodes, failed on 0 nodes.";
+
+ Assertions.assertThat(Collections.singleton(success)).containsAnyOf(outs.get(1), outs.get(2));
Review Comment:
i think it's better to assert that the list
```
assertThat(out.sublist(1, 4)).containsExactlyInAnyOrder(from, too, success)
```
you could do something similar on sublist(5,9) with a `containsExactly(success,from,to,retrieval)` assert
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSAdmin.java:
##########
@@ -1238,36 +1240,44 @@ public void testAllDatanodesReconfig()
when(reconfigurationUtil.parseChangedProperties(any(Configuration.class),
any(Configuration.class))).thenReturn(changes);
- assertEquals(0, admin.startReconfiguration("datanode", "livenodes"));
+ int result = admin.startReconfiguration("datanode", "livenodes");
+ Assertions.assertThat(result).isEqualTo(0);
final List<String> outsForStartReconf = new ArrayList<>();
final List<String> errsForStartReconf = new ArrayList<>();
reconfigurationOutErrFormatter("startReconfiguration", "datanode",
"livenodes", outsForStartReconf, errsForStartReconf);
- assertEquals(3, outsForStartReconf.size());
- assertEquals(0, errsForStartReconf.size());
- assertTrue(outsForStartReconf.get(0).startsWith("Started reconfiguration task on node"));
- assertTrue(outsForStartReconf.get(1).startsWith("Started reconfiguration task on node"));
- assertEquals("Starting of reconfiguration task successful on 2 nodes, failed on 0 nodes.",
- outsForStartReconf.get(2));
+ String started = "Started reconfiguration task on node";
+ String starting =
+ "Starting of reconfiguration task successful on 2 nodes, failed on 0 nodes.";
+ Assertions.assertThat(outsForStartReconf.size()).isEqualTo(3);
Review Comment:
do an assert that the collection is of a given size, e.g
```
assertThat(outsForStartReconf).hasSize(3)
```
the generated assert will then include the list of entries
--
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