CASSNDRA-18780 Fixed ConnectionBurnTest fails with NullPointerException #2613

Mmuzaf · 2023-08-19T17:31:08Z

Thanks for sending a pull request! Here are some tips if you're new here:

Ensure you have added or run the appropriate tests for your PR.
Be sure to keep the PR description updated to reflect all changes.
Write your PR title to summarize what this PR proposes.
If possible, provide a concise example to reproduce the issue for a faster review.
Read our contributor guidelines
If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

belliottsmith · 2023-10-27T14:55:58Z

test/burn/org/apache/cassandra/net/ConnectionBurnTest.java

        {
            Reporters reporters = new Reporters(endpoints, connections);
+            List<Exception> exceptions = new LinkedList<>();


why a linked list?

also, consider declaring more proximal to usage

This is just a local buffer to fetch results from the unexpectedExceptions queue, so since it is only used locally it should be safe to use non-thread-safe collection, or did you mean something else?

I think we can tie drainUninterruptibly and this exceptions buffer, so that they use the same buffer size via constant. I set it to 1 since we are only interested in the first exception that occurred, but we could increase it if you'd like.

I meant that LinkedList is almost always inferior to ArrayList, and there appears to be no reason to prefer it here. It's not an important point, particularly here the distinction is unimportant. Thanks for making the change, though I'm not sure why Lists.newArrayList is preferred here to simply new ArrayList<>().

Regarding declaring it more proximal to usage, I meant I don't see why it is declared at the start of the method rather than above the loop that uses it. It's helpful IMO to declare variables close to where they are logically introduced in the code.

belliottsmith · 2023-10-27T15:08:21Z

test/burn/org/apache/cassandra/net/ConnectionBurnTest.java

@@ -467,26 +491,42 @@ public void run() throws ExecutionException, InterruptedException, NoSuchFieldEx
                    }
                });

-                while (deadline > nanoTime() && failed.getCount() > 0)
+                int added = 0;


at most we need a boolean, but I'd say this should be a part of the loop guard; perhaps use a do/while loop and simply check 0 == Queues.drainUninterruptibly()

Good suggestion, fixed.

belliottsmith · 2023-10-27T15:08:28Z

test/burn/org/apache/cassandra/net/ConnectionBurnTest.java

                {
                    reporters.update();
                    reporters.print();
-                    Uninterruptibles.awaitUninterruptibly(failed, 30L, TimeUnit.SECONDS);
+                    added = Queues.drainUninterruptibly(unexpectedExceptions, exceptions, 10, 30L, TimeUnit.SECONDS);


why 10 items, rather than 1?

Fixed, and move this value to a constant value for visibility.

belliottsmith · 2023-10-27T15:11:47Z

test/burn/org/apache/cassandra/net/ConnectionBurnTest.java

                }

                executor.shutdownNow();
                ExecutorUtils.awaitTermination(5L, TimeUnit.MINUTES, executor);
+
+                if (!exceptions.isEmpty())
+                    throw exceptions.get(0);


should probably addSuppressed any items > index 0

belliottsmith · 2023-10-27T15:12:23Z

test/burn/org/apache/cassandra/net/Verifier.java

                    WaitQueue.Signal signal = writerWaiting.register();
                    if (null != (e = chunkList.firstEntry()) && chunkSequenceId - e.getKey() > 1 << 12)
-                        signal.await();
+                        signal.await(5L, TimeUnit.SECONDS);


why is this now timed?

This is for safety's sake and in accordance with the following comment which gave me the feeling that we'd get the next chunk within a reasonable timeframe (5 seconds), and if not - we could fail the test, but of course I could be missing something.

https://github.com/apache/cassandra/blob/trunk/test/burn/org/apache/cassandra/net/Verifier.java#L668

// TODO: even 2s or 5s are unreasonable periods of time without _any_ movement on a message waiting to arrive // this seems to happen regularly on MacOS, but we should confirm this does not happen on Linux

Sure, ok. Though given the earlier comment might be worth making this timeout a bit bigger to avoid false positives, or perhaps condition the wait time on the system under test.

Mmuzaf · 2023-10-31T13:17:53Z

@belliottsmith I've addressed your comments, please, take a look again :-)

Mmuzaf · 2023-11-14T17:31:40Z

@belliottsmith Hi Benedict, have you had a chance to look at the comments that have been fixed? :-)

Mmuzaf · 2023-12-19T16:51:16Z

@belliottsmith Hi Benedict, I think we are really close to fixing it! :-) Would you mind taking one last look?

belliottsmith · 2024-02-01T13:38:45Z

test/burn/org/apache/cassandra/net/ConnectionBurnTest.java

@@ -467,26 +493,43 @@ public void run() throws ExecutionException, InterruptedException, NoSuchFieldEx
                    }
                });

-                while (deadline > nanoTime() && failed.getCount() > 0)
+                while (deadline > nanoTime() &&
+                        0 == Queues.drainUninterruptibly(unexpectedExceptions,


I'm not sure this is the desired behaviour? It looks like the semantics of drainUninterruptibly is that it will wait for exceptionBufferCapacity before returning, whereas we want to return as soon as non-empty. This isn't super important here due to the timeout, but it is good to get this kind of thing right.

We should either wait for just 1 item, or use a different method for waiting, IMO.

Mmuzaf force-pushed the cassandra-18780 branch 6 times, most recently from 6d31acd to 16216c7 Compare August 20, 2023 20:45

Mmuzaf force-pushed the cassandra-18780 branch from eee50f9 to d2d633e Compare October 25, 2023 14:34

belliottsmith reviewed Oct 27, 2023

View reviewed changes

Mmuzaf added 7 commits October 31, 2023 12:54

CASSNDRA-18780 Fixed ConnectionBurnTest fails with NullPointerException

563b15c

code cleanup

9687b4d

revert changes not related to the fix

fcbd198

update deserialize/serialize interruption

7276214

fix exception that is thrown while the close is in progress

9365664

remove unused imports

e3a2f7c

Fix review comments

4e74a34

Mmuzaf force-pushed the cassandra-18780 branch from d2d633e to 4e74a34 Compare October 31, 2023 13:08

belliottsmith reviewed Feb 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CASSNDRA-18780 Fixed ConnectionBurnTest fails with NullPointerException #2613

CASSNDRA-18780 Fixed ConnectionBurnTest fails with NullPointerException #2613

Mmuzaf commented Aug 19, 2023

belliottsmith Oct 27, 2023

belliottsmith Oct 27, 2023

Mmuzaf Oct 31, 2023

belliottsmith Feb 1, 2024 •

edited

belliottsmith Oct 27, 2023

Mmuzaf Oct 31, 2023

belliottsmith Oct 27, 2023

Mmuzaf Oct 31, 2023

belliottsmith Oct 27, 2023

Mmuzaf Oct 31, 2023

belliottsmith Oct 27, 2023

Mmuzaf Oct 31, 2023

belliottsmith Feb 1, 2024

Mmuzaf commented Oct 31, 2023

Mmuzaf commented Nov 14, 2023

Mmuzaf commented Dec 19, 2023

belliottsmith Feb 1, 2024 •

edited

CASSNDRA-18780 Fixed ConnectionBurnTest fails with NullPointerException #2613

Are you sure you want to change the base?

CASSNDRA-18780 Fixed ConnectionBurnTest fails with NullPointerException #2613

Conversation

Mmuzaf commented Aug 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belliottsmith Feb 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mmuzaf commented Oct 31, 2023

Mmuzaf commented Nov 14, 2023

Mmuzaf commented Dec 19, 2023

belliottsmith Feb 1, 2024 • edited

Choose a reason for hiding this comment

belliottsmith Feb 1, 2024 •

edited

belliottsmith Feb 1, 2024 •

edited