fixes multiple tests in ShellServerIT#3785
Conversation
Multiple tests in ShellServerIT only ran when their table id was 1 or 2. This commit updates the test to run with any table id.
ctubbsii
left a comment
There was a problem hiding this comment.
The test changes are fine overall, but I think it might be easier, and have more rigid coverage if we check each line of the output, rather than do regexes to pattern match.
I also have reservations about the name of the shell command, because the name of the command is not very self-descriptive. It may be out of scope of this issue, but I think it goes to the heart of how we communicate this capability consistently to users. We're basically creating a vocabulary for users that will impact how they think about and interact with the feature, and I'm not sure about "setgoal". Even just "settabletgoalstate" is probably better in spite of its verbosity. We can discuss further on the mailing list or as we tweak the design docs, though. It need not hold up this PR.
| assertTrue(result.contains("TABLE: " + tableName[0])); | ||
| assertTrue(result.contains("TABLET ID HOSTING GOAL")); | ||
| assertTrue(result.contains("1<< ONDEMAND")); | ||
| assertTrue(result.matches("(?s).*" + tableId + "<<\\s+ONDEMAND.*")); |
There was a problem hiding this comment.
Are the lines in the output coming in an unpredictable order? Is there a reason why we're doing pattern matching anywhere in the multi-line string result for the lines we expect, instead of just iterating over the lines and checking to verify each line contains the next thing we expect?
Something like:
Iterator<String> iter = splitLines(result);
line = iter.next(); assertTrue(line.contains(firstLineCondition));
line = iter.next(); assertTrue(line.contains(nextLineCondition));
line = iter.next(); assertTrue(line.contains(nextLineCondition));
// ...
assertFalse(iter.hasNext());This seems simpler to read and maintain than a bunch of regexes that don't verify that the output is appearing in the expected order.
There was a problem hiding this comment.
The reason I did pattern matching was because the number of spaces in the output would change when the length of the table id changed. So for a one character table id would see more spaces in the output than if there was a two digit table id.
As for checking the order, that could be a follow on issue. It would make the test more strict. The test was not checking the order before these changes.
| ts.exec("addsplits -t " + table + " a c e g m t"); | ||
| ts.exec("goal -t " + table + " -b a -e a -g never"); | ||
| ts.exec("goal -t " + table + " -b c -e e -ee -g always"); | ||
| ts.exec("setgoal -t " + table + " -b a -e a -g never"); |
There was a problem hiding this comment.
I think setgoal is not a very good name for a tablet-specific function, as it makes one wonder "goal for what?", because "goal" is too generic of a term.
I think "hosttablet ... always/never/ondemand" or "settabletgoal ... always/never/ondemand". Even these are probably not the best names either, but they give a lot more context to what kind of goal is being set.
Alternatively, one could have a command named "tablet" and a sub-command called something like "online", so you get a very expressive command like: "tablet online... always/never/ondemand" or maybe "tablet hosted... always/never/ondemand".
Or we could overload the current online command to be able to set a range, and an optional flag for ondemand. So, you get the following:
> online -t table -b beginRange -e endRange # "always"
> offline -t table -b beginRange -e endRange # "never"
> online -t table -b beginRange -e endRange --ondemand # "ondemand"There was a problem hiding this comment.
Elsewhere, we call this "hosting goal", so "sethostinggoal" would be appropriate as well.
There was a problem hiding this comment.
@ctubbsii the name of that command is out of scope for this PR, but what you wrote sounds reasonable. Can you open an issue for reconsidering the name?
…T.java Co-authored-by: Christopher Tubbs <ctubbsii@apache.org>
Multiple tests in ShellServerIT only ran when their table id was 1 or 2. This commit updates the test to run with any table id.