Skip to content

Conversation

@rangadi
Copy link
Contributor

@rangadi rangadi commented May 5, 2016

  • after shutting down, wait for the threads to actually finish.
  • handle a possible race between poll thread and close().

 - handle a possible race between poll thread and close().
@rangadi rangadi changed the title - after shutting down consumer poll thread (and offset updater thread), wait for them to actually exit. fix ConcurrentModificationException in KafkaIO May 5, 2016
@rangadi
Copy link
Contributor Author

rangadi commented May 5, 2016

R: @dhalperi & @tgroh . Please take a look when you get a chance.

&& offsetFetcherThread.awaitTermination(10, TimeUnit.SECONDS)) {
break; // done
}
} catch (InterruptedException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should you set the interrupted bit here?

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 set the flag in two places where we are interrupted on thread not in our control.

@dhalperi
Copy link
Contributor

dhalperi commented May 5, 2016

  • Can you confirm that you've run this with the InProcessPipelineRunner and the prior issues are not present?

LGTM once box is checked and tests pass. If you've done both and I haven't merged, ping me ;)

@rangadi
Copy link
Contributor Author

rangadi commented May 11, 2016

R: @dhalperi , I could reproduce this by adding an extra cosumer.poll() after 'closed' flag was seen in consumerPollLoop(). I could not reproduce the problem without extra poll.

With that, verified both that the fix works and in case the thread takes much longer, close() properly waits for it to shutdown with periodic warning log.

Raghu.

@dhalperi
Copy link
Contributor

LGTM, will merge soon.

@asfgit asfgit closed this in 7108bc0 May 11, 2016
iemejia pushed a commit to iemejia/beam that referenced this pull request Jan 12, 2018
pl04351820 pushed a commit to pl04351820/beam that referenced this pull request Dec 20, 2023
Adds a system test demonstrating that ArrayUnion operation no longer deletes pre-existing data

Fixes apache#14 🦕
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants