ndimiduk commented on code in PR #105:
URL:
https://github.com/apache/hbase-operator-tools/pull/105#discussion_r847490714
##########
hbase-hbck2/src/test/java/org/apache/hbase/TestHBCK2.java:
##########
@@ -121,26 +125,62 @@ public void testSetTableStateInMeta() throws IOException {
// Restore the state.
state = this.hbck2.setTableState(hbck, TABLE_NAME, state.getState());
assertTrue("Found=" + state.getState(), state.isDisabled());
+
+ // Test the new method with arg list
+ String[] args = new String[]{TABLE_NAME.getNameAsString(), "DISABLED"};
+ state = this.hbck2.setTableStateByArgs(hbck, args);
+ assertTrue("Found=" + state.getState(), state.isEnabled());
+ }
+ }
+
+ @Test
+ public void testSetTableStateWithInputFiles() throws IOException {
+ File testFile = new File(TEST_UTIL.getDataTestDir().toString(),
"inputForSetTableTest");
Review Comment:
I'm thinking about the case when the test is run twice without a clean in
between. Can these temporary files conflict in some way? What happens when this
output path exists, does the test overwrite it? I don't think there's any kind
of test run uniqueness implemented in `TEST_UTIL.getDataTestDir()`, but I don't
know for sure. Do you mind taking a look into this? -- we want to avoid flakey
tests wherever possible.
##########
hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java:
##########
@@ -185,6 +185,37 @@ void checkFunctionSupported(ClusterConnection connection,
String cmd) throws IOE
}
}
+ void setTableState(Hbck hbck, String[] args) throws IOException {
+ Options options = new Options();
+ Option inputFile = Option.builder("i").longOpt("inputFiles").build();
+ options.addOption(inputFile);
+ CommandLine commandLine = getCommandLine(args, options);
+ if (commandLine == null) {
+ return;
+ }
+ String[] argList = commandLine.getArgs();
+ if(!commandLine.hasOption(inputFile.getOpt())) {
+ System.out.println(setTableStateByArgs(hbck, argList));
+ } else {
+ List<String> inputList = getFromArgsOrFiles(stringArrayToList(argList),
true);
+ for (String line : inputList) {
+ String[] params = line.split("\\s+");
Review Comment:
Ah, okay.
##########
hbase-hbck2/src/test/java/org/apache/hbase/TestHBCKCommandLineParsing.java:
##########
@@ -61,16 +83,21 @@ public void testErrorMessage() throws IOException{
@Test (expected=NumberFormatException.class)
public void testCommandWithOptions() throws IOException {
- HBCK2 hbck = new HBCK2(TEST_UTIL.getConfiguration());
// The 'x' below should cause the NumberFormatException. The Options
should all be good.
- hbck.run(new String[]{"bypass", "--lockWait=3", "--override",
"--recursive", "x"});
+ this.hbck2.run(new String[]{"bypass", "--lockWait=3", "--override",
"--recursive", "x"});
+ }
+
+ @Test (expected=FileNotFoundException.class)
+ public void testInputFileOption() throws IOException {
+ // The 'x' below should cause the io exception for file not found.
+ // The Options should all be good.
+ this.hbck2.run(new String[]{"bypass", "--override", "--inputFile", "x"});
Review Comment:
Ah, okay. here you run a test using the long option. That's fine with me,
just so long as there's coverage of both modes of user interaction.
##########
hbase-hbck2/src/test/java/org/apache/hbase/TestHBCK2.java:
##########
@@ -287,21 +359,39 @@ public void
testFormatReportMissingRegionsInMetaNoMissing() throws IOException {
public void testFormatReportMissingInMetaOneMissing() throws IOException {
TableName tableName = createTestTable(5);
List<RegionInfo> regions = HBCKMetaTableAccessor
- .getTableRegions(TEST_UTIL.getConnection(), tableName);
+ .getTableRegions(TEST_UTIL.getConnection(), tableName);
HBCKMetaTableAccessor.deleteRegionInfo(TEST_UTIL.getConnection(),
regions.get(0));
String expectedResult = "Missing Regions for each table:\n";
String result = testFormatMissingRegionsInMetaReport();
//validates initial report message
assertTrue(result.contains(expectedResult));
//validates our test table region is reported missing
expectedResult = "\t" + tableName.getNameAsString() + "->\n\t\t"
- + regions.get(0).getEncodedName();
+ + regions.get(0).getEncodedName();
assertTrue(result.contains(expectedResult));
//validates namespace region is not reported missing
expectedResult = "\n\thbase:namespace -> No mismatching regions. This
table is good!\n\t";
assertTrue(result.contains(expectedResult));
}
+ private void writeStringsToAFile(File testFile, String[] strs) throws
IOException {
+ try (FileOutputStream output = new FileOutputStream(testFile, false)) {
+ for (String regionStr : strs) {
+ output.write((regionStr + System.lineSeparator()).getBytes());
+ }
+ }
+ }
+
+ private List<Long> getPidsFromResult(String result) {
+ Scanner scanner = new Scanner(result).useDelimiter("[\\D]+");
+ List<Long> pids = new ArrayList<>();
+ while (scanner.hasNext()) {
+ pids.add(scanner.nextLong());
+ }
+ scanner.close();
Review Comment:
Can you manage the `scanner` instance using try-with-resource?
##########
hbase-hbck2/src/test/java/org/apache/hbase/TestHBCK2.java:
##########
@@ -121,26 +125,62 @@ public void testSetTableStateInMeta() throws IOException {
// Restore the state.
state = this.hbck2.setTableState(hbck, TABLE_NAME, state.getState());
assertTrue("Found=" + state.getState(), state.isDisabled());
+
+ // Test the new method with arg list
+ String[] args = new String[]{TABLE_NAME.getNameAsString(), "DISABLED"};
+ state = this.hbck2.setTableStateByArgs(hbck, args);
+ assertTrue("Found=" + state.getState(), state.isEnabled());
+ }
+ }
+
+ @Test
+ public void testSetTableStateWithInputFiles() throws IOException {
+ File testFile = new File(TEST_UTIL.getDataTestDir().toString(),
"inputForSetTableTest");
+ writeStringsToAFile(testFile, new String[]{TABLE_NAME.getNameAsString() +
" DISABLED"});
+ String result = testRunWithArgs(new String[]{SET_TABLE_STATE, "-i",
testFile.toString()});
+ assertTrue(result.contains("tableName=TestHBCK2, state=ENABLED"));
+
+ // Restore the state.
+ try (ClusterConnection connection = this.hbck2.connect(); Hbck hbck =
connection.getHbck()) {
+ TableState state = this.hbck2.setTableState(hbck, TABLE_NAME,
TableState.State.ENABLED);
+ assertTrue("Found=" + state.getState(), state.isDisabled());
+ }
+ }
+
+ @Test
+ public void testUnAssigns() throws IOException {
+ try (Admin admin = TEST_UTIL.getConnection().getAdmin()) {
+ List<RegionInfo> regions = admin.getRegions(TABLE_NAME);
+ for (RegionInfo ri : regions) {
+ RegionState rs =
TEST_UTIL.getHBaseCluster().getMaster().getAssignmentManager().
+ getRegionStates().getRegionState(ri.getEncodedName());
+ LOG.info("RS: {}", rs.toString());
+ }
+ String[] regionStrsArray =
+
regions.stream().map(RegionInfo::getEncodedName).toArray(String[]::new);
+ File testFile = new File(TEST_UTIL.getDataTestDir().toString(),
"inputForUnAssignsTest");
+ writeStringsToAFile(testFile, regionStrsArray);
+ String result = testRunWithArgs(new String[]{UNASSIGNS, "-i",
testFile.toString()});
Review Comment:
Maybe have one of these tests use the long option (`--inputFiles`) as well?
##########
hbase-hbck2/src/main/java/org/apache/hbase/HBCK2.java:
##########
@@ -1202,4 +1318,70 @@ public static void main(String [] args) throws Exception
{
System.exit(errCode);
}
}
+
+ private List<String> stringArrayToList(String... nameSpaceOrTable) {
+ return nameSpaceOrTable != null ? Arrays.asList(nameSpaceOrTable) : null;
+ }
+
+ /**
+ * Get list of input if no other options
+ * @param args Array of arguments
+ * @return the list of input from arguments or parsed from input files
+ */
+ private List<String> getInputList(String[] args) throws IOException {
+ if (args == null) {
+ return null;
Review Comment:
Yikes, really. Okay...
--
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]
