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

Make RecordStreamSourceImpl threadsafe #114

Merged
merged 1 commit into from
Dec 21, 2021

Conversation

remcowesterhoud
Copy link
Contributor

@remcowesterhoud remcowesterhoud commented Dec 20, 2021

Description

I've added a synchronized around the updating of the list of records in the record stream. I've written a test to verify this change solved the issue in #110

Related issues

closes #110

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually

Documentation:

  • Javadoc has been written
  • The documentation is updated

Copy link
Contributor

@pihme pihme left a comment

Choose a reason for hiding this comment

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

Looks good! 🎉

There is one minor change. If you only change that, I don't need to see it again before merge.
There is also a suggestion for improving the test. If you follow that suggestion, I would like to see the changed test again.

Also happy with merging it now, releasing the bugfix and having a follow up issue to improve the test.

import org.junit.jupiter.api.Test;

@ZeebeProcessTest
public class MultiThreadTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 I am of two minds about this test. On the one hand I am glad there is a test at all. But this test looks like it might be a flaky test. It feels too short, and I could imagine even with the wrong implementation it will sometimes pass and sometimes fail. I might be wrong though.

Also, I don't really have a better idea.

There is the option of using jqwik and having a number of runs with a random number of runners and each looping a random number of times with a random operation. But at the same time this feels like overkill.

Still, if you feel like making this more solid, you could have a look at: https://github.com/camunda-cloud/zeebe/blob/develop/atomix/cluster/src/test/java/io/atomix/raft/RandomizedRaftTest.java or https://github.com/camunda-cloud/zeebe/blob/develop/broker/src/test/java/io/camunda/zeebe/broker/system/partitions/impl/RandomizedPartitionTransitionTest.java

None of them are clean examples of property based tests. I am not saying you need to change this test. But if you either feel uncomfortable about this test and/or are curious about property based testing, you might give this a go

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 understand your concern. However, the test didn't seem flaky when I was playing around with it. Without the fix it failed 100% of the runs, and with the fix I haven't seen it fail yet.

I would propose to keep it as is and if it turns out to be flaky and becomes a problem we can fix it at that time.

- Synchronized updating of records list
@remcowesterhoud remcowesterhoud merged commit 39e8ca0 into main Dec 21, 2021
@remcowesterhoud remcowesterhoud deleted the 110-threadsafe-recordstreamsource branch December 21, 2021 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants