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

branch-3.4 -- bugfix -- ZOOKEEPER-2894 #363

Closed

Conversation

@xoiss
Copy link
Contributor

xoiss commented Sep 8, 2017

@phunt

This comment has been minimized.

Copy link
Contributor

phunt commented Sep 28, 2017

Hi @xoiss please update this PR to follow the guidelines in "generate a pull request" here: https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute - in particular wrt the summary and description text which really helps folks identifying which PRs to prioritize. Thanks!

@anmolnar

This comment has been minimized.

Copy link
Contributor

anmolnar commented Jan 30, 2019

@xoiss Looks like a very nice catch. Would you mind creating a separate pull request for the master branch that we can commit first and backport to other branches?

@anmolnar

This comment has been minimized.

Copy link
Contributor

anmolnar commented Feb 7, 2019

Closing, because @xoiss hasn't been responding for a while. Please re-open if you can come back to this.

@anmolnar anmolnar closed this Feb 7, 2019
@xoiss

This comment has been minimized.

Copy link
Contributor Author

xoiss commented Feb 7, 2019

// xoiss is here

hi, all!
unfortunately i don't have enough time to respond promptly
and moreover i don't check the gmail mail box because of... some reason

ok, what we can do with these?

@anmolnar

This comment has been minimized.

Copy link
Contributor

anmolnar commented Feb 7, 2019

@xoiss Same story, please rebase.

@anmolnar anmolnar reopened this Feb 7, 2019
@xoiss

This comment has been minimized.

Copy link
Contributor Author

xoiss commented Feb 8, 2019

Ok... As I remember, it turned out there is bug in the fix proposed by me here in case of multi-threaded build. So, it remedies the single-threaded build, but breaks the multi-threaded.

The cause is the 'assert' in the else part. I don't remember what was the idea to put it in the else part... to guarantee what. Surely, the else part never happens in the single-threaded version. But, it occurs in 100% of cases in the multi-threaded, and process gets SIGABRT. As soon as I've been using only the single-treaded version those times, I did not notice that in September 2017. We noticed it later with the different project that uses multi-threaded.

Ok, we have already debugged this and refixed in our locally patched fork, but I have not updated this request here.

So, I need to rework and rebase.

@anmolnar

This comment has been minimized.

Copy link
Contributor

anmolnar commented Mar 20, 2019

ping @xoiss

@xoiss

This comment has been minimized.

Copy link
Contributor Author

xoiss commented Jun 23, 2019

Hello!
I have revised the issue description, and also I have fixed and reworked this PR.

The improved issue: https://issues.apache.org/jira/browse/ZOOKEEPER-2894
Essentially, I left it unchanged. I have read it now and I've found it more-or-less clear.
Surely, it's very huge, but if one reads it with attention, I think she-or-he would get the idea.
The only thing, I emphasized subparts, where is the problem description, and where is the proposal.

The renewed PR: #1000
// hey, look! it's the number 1000 :)
I have rebased it on the latest branch-3.4.
As I can see, it passed all precommit checks.
And the main thing, I've remedied my patch for the case of multi-threaded build.

So, I hope now it comply with the PR-style.

Thank you!
Alex (aka xoiss)

Please, revise my renewed PR: #1000

@xoiss

This comment has been minimized.

Copy link
Contributor Author

xoiss commented Jun 23, 2019

pong @anmolnar
;]

@anmolnar

This comment has been minimized.

Copy link
Contributor

anmolnar commented Jul 9, 2019

Closing because #1000 has been merged.

@anmolnar anmolnar closed this Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.