CURATOR-719: Fix orSetData for parallel create calls#510
CURATOR-719: Fix orSetData for parallel create calls#510eolivelli merged 1 commit intoapache:masterfrom
Conversation
HoustonPutman
force-pushed
the
parallel-creates
branch
from
73e1cb8 to
85fd82d
Compare
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
Overall LGMT
but maybe the new test may pass not reproduce the issue
| } | ||
| return null; | ||
| }; | ||
| Future<Exception> f1 = executor.submit(setData); |
There was a problem hiding this comment.
we may be very lucky and not reproduce the problem (if the machine is "slow" and the two operations don't run concurrently)
maybe we can make this a little bit nasty and schedule many operations in parallel.
It would be great to have a way to intercept the code paths and verify that we have exercised all the possible branches. (Mockito?)
There was a problem hiding this comment.
we may be very lucky and not reproduce the problem (if the machine is "slow" and the two operations don't run concurrently)
For concurrent sensitive test, I usually run with help from IDE "until failure" or "run x times". I, personally, think "reproduce" means that it will reproduce the bug given enough runs(tens to hundreds depends on context).
Mockito?
I prefer to run multiple times over mock if the number of runs is small. Mock forces us to think different and complicate things.
I tested this after reverting the fix, it failed in the first runs without resorting to "run x times" on my 2015 macbook pro.
There was a problem hiding this comment.
with Mockito I mean that we can use a spy or override some method to ensure that we are hitting the expected code path.
I tested this after reverting the fix, it failed in the first runs without resorting to "run x times" on my 2015 macbook pro
let me try
|
thank you very much @HoustonPutman |
|
Thanks for checking y'all! I did make sure it failed without the fix, but I know how pesky these types of tests can be on different hardware and runtimes.
I will definitely have a few more in the next few weeks 🙂 |
3 participants
https://issues.apache.org/jira/browse/CURATOR-719
This fix really just lets the
catch (KeeperException.NodeExistsException e)clause catch exceptions thrown in theif (createParentsIfNeeded)clause. Previously, since the twocatchstatements were for the same try, theKeeperException.NodeExistsExceptionlogic would not be called if that error was thrown when creating the node after creating the parents. This case can easily be seen when trying to call create.orSetData() in parallel on the same node, while creating parents if needed. This is because theNoNodeExceptionwould be thrown for both parallel calls, but only one would be able to create the new node in the catch statement, and the other would get an un-catchableNodeExistsException.