Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CASSANDRA-18753 followup: Further test fixes #3165

Closed
wants to merge 4 commits into from
Closed

CASSANDRA-18753 followup: Further test fixes #3165

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
01b0d20
DO NOT COMMIT: Circle CI config
blambov Mar 7, 2024
d6b08b5
Additional fix for NPE when initializing CFS (CASSANDRA-19169)
blambov Mar 7, 2024
49464ff
Change BounceIndexRebuildTest to work with SAI as default index
blambov Mar 7, 2024
c485840
Code Review: Change BounceIndexRebuildTest to verify index is not reb…
blambov Mar 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion test/distributed/org/apache/cassandra/distributed/test/log/BounceIndexRebuildTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,12 @@ public void bounceTest() throws Exception
cluster.schemaChange(withKeyspace("create index idx on %s.tbl (x)"));
Object[][] res = cluster.coordinator(1).execute(withKeyspace("select * from %s.tbl where x=5"), ConsistencyLevel.ALL);
assert res.length > 0;
final String pattern = "Index build of idx complete|Index \\[idx\\] became queryable after successful build";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while since I looked at this, so I had to code dive a bit to understand why switching between legacy 2i and SAI would necessitate this change. Would you be ok with adding a comment? Something along the lines of:

// Both SAI and legacy 2i indexes will log the "Index [idx] became queryable..." message every time the index is 
// initialised. Only legacy indexes may rebuild a previously built index, in which case they log the "Index
// build of idx complete" message. So this is to verify that happens at most once, implying that legacy
// 2i does not get rebuilt after a restart.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can see, though, it does get rebuilt after the restart. With legacy index the pre-bounce count is 0 and it becomes 1 after the restart. With SAI the numbers are 1 and 2, respectively.

I really don't know what the test is supposed to verify; it is introduced with TCM and I went with the understanding that the test is checking that there is a rebuild after the restart.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can see, though, it does get rebuilt after the restart. With legacy index the pre-bounce count is 0 and it becomes 1 after the restart.

Are you sure? What I see on trunk is that the pre-bounce count is 1 and stays at 1 following the restart.
If we change the search to use your pattern that changes to 2 pre-bounce and 3 after, because we always log the Index [idx] became queryable message, whether the index was rebuilt or not.

With SAI the numbers are 1 and 2, respectively

Yes, because both SAI and legacy 2i indexes will log the Index [idx] became queryable... as I said in the comment, but SAI will never log Index build of idx complete.

I really don't know what the test is supposed to verify; it is introduced with TCM and I went with the understanding that the test is checking that there is a rebuild after the restart.

er, no it's the opposite. It's verifying that the legacy 2i index is not rebuilt after a bounce (which would cause it to log Index build of idx complete).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. What I needed, then, is to identify the corresponding message for SAI.

There does not appear to be one, though, but it does show "Validating" after restart. Changed test to check that if no legacy build message was reported, then a SAI validation one must be seen post-bounce.

int preBounce = cluster.get(1).logs().grep(pattern).getResult().size();

cluster.get(1).shutdown().get();
cluster.get(1).startup();
assertEquals(1, cluster.get(1).logs().grep("Index build of idx complete").getResult().size());
assertEquals(preBounce + 1, cluster.get(1).logs().grep(pattern).getResult().size());
res = cluster.coordinator(1).execute(withKeyspace("select * from %s.tbl where x=5"), ConsistencyLevel.ALL);
assert res.length > 0;
}
Expand Down