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
HBASE-23601: OutputSink.WriterThread exception gets stuck and repeated indefinietly #956
Conversation
…d indefinietly clear exception after logged try to restart writer threads if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add a test that reproduces the problem?
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good find.Left a few notes.
for (WriterThread t : writerThreads) { | ||
if (!t.isAlive()){ | ||
LOG.debug("restarting thread" + t); | ||
t.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think you could restart a dead thread. You have to create a new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely. Thanks for pointing it out.
|
||
public Throwable clearError(){ | ||
return thrown.getAndSet(null); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we make the pipelinecontroller once and use it thereafter -- it goes down into the WriterThread.... . I was going to suggest just create a new Controller on each write but looks like too much of a refactor.
Would it be cleaner if checkForErrors cleared any errors found... ; i.e. checkForErrors throws but before it throws it clears any exception in Controller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not just cleaner it would also cover more usecases. I moved that line into the checkForErrors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this problem only occur in HBase 2.2.x? If not it would be better to target the master branch.
…d indefinietly remove dead writer threads and create new ones reusing the old names remove the stored exception before re-throwing it add unit test
…d indefinietly add missing changes from prev commit
@wchevreuil > Is it possible to add a test that reproduces the problem? @HorizonNet > Does this problem only occur in HBase 2.2.x? If not it would be better to target the master branch. |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Few notes.
while(writerIterator.hasNext()){ | ||
WriterThread t = writerIterator.next(); | ||
if (!t.isAlive()){ | ||
names.add(t.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be easier to move through the writerThread array using an index replacing dead threads with new live ones as you go? Should log when you replace a thread I'd say at DEBUG level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. Added it to the next commit.
this.entryBuffers = entryBuffers; | ||
outputSink = sink; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have the constructor just above this call this one passing the constructed thread name? nit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
…d indefinietly fix checkstyle validations add license to test class code improvements
🎊 +1 overall
This message was automatically generated. |
…d indefinietly (#956) * HBASE-23601: OutputSink.WriterThread exception gets stuck and repeated indefinietly clear exception after logged try to restart writer threads if needed
…d indefinietly (#956) * HBASE-23601: OutputSink.WriterThread exception gets stuck and repeated indefinietly clear exception after logged try to restart writer threads if needed
…d indefinietly (apache#956) * HBASE-23601: OutputSink.WriterThread exception gets stuck and repeated indefinietly clear exception after logged try to restart writer threads if needed
created a new PR for branch-2 #1028 |
…d indefinietly (apache#956) * HBASE-23601: OutputSink.WriterThread exception gets stuck and repeated indefinietly clear exception after logged try to restart writer threads if needed (cherry picked from commit ce7c559) Change-Id: I62ede7ebc02213357678e50b9b995b00dfac4a5d
… repeated indefinietly (apache#956)" This reverts commit ce7c559. (cherry picked from commit fd6eb38) Change-Id: I67ab28228bec55a17d2b8c01946be264723ad2a6
clear exception after logged
try to restart writer threads if needed