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.

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

CASSANDRA-18753 followup: Further test fixes #3165

Closed
wants to merge 4 commits into from

Conversation

blambov
Copy link
Contributor

@blambov blambov commented Mar 7, 2024

Fix dtest failures caused by an omitted part of the fix to avoid NPE on UCS initialization (CASSANDRA-19169).
Fix BounceIndexRebuildTest for SAI.

@blambov blambov changed the title CASSANDRA-18753 followup: Further fix to avoid NPE on UCS initialization CASSANDRA-18753 followup: Further test fixes Mar 7, 2024
@blambov
Copy link
Contributor Author

blambov commented Mar 7, 2024

@beobal could you review these short test fixes?

@beobal
Copy link
Contributor

beobal commented Mar 7, 2024

@beobal could you review these short test fixes?

Sure, will do

Copy link
Contributor

@beobal beobal left a comment

Choose a reason for hiding this comment

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

LGTM. I just made a minor suggestion for an explanatory comment on the index rebuild test, feel free to ignore that if you think it's too obvious.

@@ -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.

Copy link
Contributor

@beobal beobal left a comment

Choose a reason for hiding this comment

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

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants