Considers all tablet metadata columns in split code #4323
Conversation
Made the following changes to the split code that adds new tablets and updates the existing tablet. * fixed potential NPE w/ tablet operation id check by reversing order of equals check * Throws IllegalStateException when attempting to split tablet with merged or cloned markers * Removed adding wals when creating new tablets in split, its not expected that the parent tablet would have wals and this is checked earlier * Deleted any user compaction requested, hosting requested, suspended, or last columns in the parent tablet Added a unit test that attempts to exercise the split code with all tablet columns. The unit test also has a set of tablet columns that were verified to work with split and it is checked against the set of columns in the code. The purpose of this test is to fail when a new column is added to ensure that split is considered. Was a bit uncertain about deleting the last location and suspend. Those columns either need to be deleted from the parent tablet or added to the new tablets being created. The current code was doing neither. Decided to delete them as the new tablets have a different range and are conceptually different tablets than the parent.
cshannon
left a comment
cshannon
left a comment
There was a problem hiding this comment.
I am still reviewing this but one thought I had would be with all the state checks that this PR introduces (such as throwing an ISE if there's an unexpected merged marker on split) we should probably add some tests to see what happens if we encounter that situation. I'm wondering if we could get into a weird state where we just start rapidly throwing exceptions in a loop as it tries to split over and over and keeps failing so it would be good to test what the outcome is if there's unexpected columns that show up and that it's acceptable behavior for the system. The other alternative would be to clean up the unexpected markers instead of the state check but if we end up with columns that should never exist then forcing manual intervention to investigate probably is better so I think the the state checks probably make sense.
|
This is probably something to do as a follow on when the merge test is looked at, but you could try to move common code related to mocking TabletMetadata to a central place like a utility class to be reused. My assumption is a lot of setup for all of the mocks could be re-used for testing that merge handles all the columns. It should make it easier to update the tests when adding new columns that need to be considered for both split and merge. |
| @Test | ||
| public void checkColumns() { | ||
| for (ColumnType columnType : ColumnType.values()) { | ||
| assertTrue(COLUMNS_HANDLED_BY_SPLIT.contains(columnType), |
There was a problem hiding this comment.
I don't know if this is easily possible with EasyMock but I was wondering if the mock API supports a way to check that all methods have been mocked with expectations? I suppose we could just use reflection to iterate over the methods and check each one, but I'm curious if there's a simpler way to check the mocked object has methods that haven't been mocked so we could throw an error if new methods showed up but were not mocked with a return value, which would of course imply it was not considered.
There was a problem hiding this comment.
I don't know if this is easily possible with EasyMock but I was wondering if the mock API supports a way to check that all methods have been mocked with expectations?
For this case I was kinda covering it with that set of columns reviewed by a developer in the test. Not sure how to do that in EasyMock.
It would be good to add an IT for this. Could manually add an unexpected column to a tablet in an IT and then attempt to split it. I'll add that in this PR. |
3 participants
Made the following changes to the split code that adds new tablets and updates the existing tablet.
Added a unit test that attempts to exercise the split code with all tablet columns. The unit test also has a set of tablet columns that were verified to work with split and it is checked against the set of columns in the code. The purpose of this test is to fail when a new column is added to ensure that split is considered.
Was a bit uncertain about deleting the last location and suspend. Those columns either need to be deleted from the parent tablet or added to the new tablets being created. The current code was doing neither. Decided to delete them as the new tablets have a different range and are conceptually different tablets than the parent.