HBASE-28903: Incremental backup test missing explicit test for bulkloads #6345
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
||
| // implement all test cases in 1 test since incremental | ||
| // backup/restore has dependencies | ||
| @SuppressWarnings("checkstyle:MethodLength") |
There was a problem hiding this comment.
Let me know if there's a better way to go about this. We could split the test up, but I think it's nice to have one single test for the entire backup/restore process. It helps simulate real-life scenarios where we're constantly taking backups of the same table.
There was a problem hiding this comment.
Checkstyle failures are not fatal to the build, so please don't mask them. If you think that the current length limit is too low, that's a discission for the dev list.
|
cc @DieterDP-ng going to tag you here as well :) |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| Assert.assertEquals(HBaseTestingUtil.countRows(hTable, famsToLoad), rowsExpected); | ||
| } | ||
|
|
||
| private static Path createHFiles(TableName tn, String regionName, byte[]... fams) |
There was a problem hiding this comment.
I'd avoid generating random data. Test failures may get ignored as flaky tests.
| newTable1Desc = TableDescriptorBuilder.newBuilder(newTable1Desc) | ||
| .setColumnFamily(ColumnFamilyDescriptorBuilder.of(fam3Name)).build(); | ||
| updateDesc(newTable1Desc, tables, conn); | ||
| testBulkloads(conn, famName, mobName); |
There was a problem hiding this comment.
I think it would be better to include the bulkfile generation and tests in the the above logic rather than add them at the end here. Unless you specifically want to test the behavior when a new CF was added, in that case I'd suggest to create a dedicated test method so it is clear what is being tested.
Just some background: normally each functionality would be tested in a separate test method, but because the backup-creation process takes a long time, some backup tests test multiple things at the same time. Hence they get messy real quick. It's a delicate balance.
There was a problem hiding this comment.
Maybe I'll split up the tests in that case. I just want to test that bulkloaded files actually are restored to the table correctly. So I can have a bulkload-specific test that
- Bulkloads data + writes data
- Takes a full backup
- Verifies all the data exists
- Bulkloads more data
- Takes incremental backup
- Verifies data exists
| return regionDir; | ||
| } | ||
|
|
||
| private static void verifyHFileHasNRows(Path path, int rows) throws IOException { |
There was a problem hiding this comment.
Best to check (some) actual row values as well, not just the counts. This will also be easier without random data.
There was a problem hiding this comment.
If you're up to it, feel free to refactor this test to make it more readable.
If you do so, I suggest 2 commits: one to refactor the test, one to add the bulkload tests to it.
| Assert.assertEquals(HBaseTestingUtil.countRows(hTable, famsToLoad), rowsExpected); | ||
| } | ||
|
|
||
| private static Path createHFiles(TableName tn, String regionName, byte[]... fams) |
There was a problem hiding this comment.
I'd actually expect that the TestFullBackup also needs to test bulkloads. So either a similar method to generate HFiles already exists, or this method can move to a higher level so it can be used in multiple tests.
4 participants
No description provided.