Skip to content

[BATCH-2762] Introducing SynchronizedItemStreamWriter #665

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

Closed
wants to merge 5 commits into from

Conversation

dimitrisli
Copy link
Contributor

Also introducing SynchronizedItemStreamWriterBuilder. Multi-threaded
Tests have been that additionally prove that currently ItemStreamWriter
is not thread-safe.

Also introducing SynchronizedItemStreamWriterBuilder. Multi-threaded
Tests have been that additionally prove that currently ItemStreamWriter
is not thread-safe.
@dimitrisli
Copy link
Contributor Author

The multi-threaded tests are inspired by the construct in SynchronizedItemStreamReaderTests but it follows slightly different approach:

  • using and atomic integer to track access to the write method
  • a CountDownLatch to get all the threads ready
  • assertion within the write method (closely paired with the atomic integer above)

Also realised that the existing SynchronizedItemStreamReaderTests is not actually properly working as the threads are not being started correctly - they are just invoking directly the run() method. I can fix this on a separate PR.

@fmbenhassine
Copy link
Contributor

@dimitrisli Thank you for this PR! The synchronized item writer and its builder LGTM. However, when I see the tests, it feels like we are trying to test the synchronized modifier of Java. I would not bother testing that only one thread is calling the method at a time. I really trust the JVM here.

I think calling the synchronized writer and make sure the delegate was called is already ok. I know you said you took inspiration from SynchronizedItemStreamReaderTests, but those tests could be simplified as well. I will give you more details on that in your other PR #666.

I added a couple of minor inline comments, if you can address them the PR should be good to merge. Thank you upfront.

@fmbenhassine fmbenhassine added the status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter label Apr 17, 2020
@dimitrisli dimitrisli requested a review from fmbenhassine April 18, 2020 21:13
@fmbenhassine
Copy link
Contributor

Thank you for the updates @dimitrisli !

However, when I see the tests, it feels like we are trying to test the synchronized modifier of Java. I would not bother testing that only one thread is calling the method at a time. I really trust the JVM here. I think calling the synchronized writer and make sure the delegate was called is already ok.

Do you agree on simplifying tests? I did not request a change for that (I can do it when merging your PR unless you want to push a second update), but at least I wanted to know if you agree on that.

@dimitrisli
Copy link
Contributor Author

@benas totally agree. I've removed the concurrency tooling and focused on the delegate pass-through methods calling

@fmbenhassine fmbenhassine removed the status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter label May 13, 2020
@fmbenhassine
Copy link
Contributor

@dimitrisli Great! LGTM now. Rebased, squashed and merged as 117b7d5. Please note that I applied some minor changes (update year in license headers, update javadoc, etc, see 9fbd3da).

Thank you for your contribution!

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

Successfully merging this pull request may close these issues.

3 participants