You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by xiaojian zhou <zh...@gmail.com> on 2015/09/22 19:50:12 UTC
Re: Review Request 38572: Fix CI failure for ShutdownAllDUnitTest
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38572/
-----------------------------------------------------------
(Updated Sept. 22, 2015, 5:50 p.m.)
Review request for geode and Darrel Schneider.
Changes
-------
Add the fix:
existingSystemDisconnected = false
Bugs: geode-329
https://issues.apache.org/jira/browse/geode-329
Repository: geode
Description
-------
There's a race conditition that 2 DS will be created when the current DS is disconnecting.
Diffs
-----
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java b7b2cd8
Diff: https://reviews.apache.org/r/38572/diff/
Testing
-------
precheckin
Thanks,
xiaojian zhou
Re: Review Request 38572: Fix CI failure for ShutdownAllDUnitTest
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38572/#review100905
-----------------------------------------------------------
Ship it!
Ship It!
- Darrel Schneider
On Sept. 28, 2015, 4:36 p.m., xiaojian zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38572/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2015, 4:36 p.m.)
>
>
> Review request for geode and Darrel Schneider.
>
>
> Bugs: geode-329
> https://issues.apache.org/jira/browse/geode-329
>
>
> Repository: geode
>
>
> Description
> -------
>
> There's a race conditition that 2 DS will be created when the current DS is disconnecting.
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java b7b2cd8
>
> Diff: https://reviews.apache.org/r/38572/diff/
>
>
> Testing
> -------
>
> precheckin
>
>
> Thanks,
>
> xiaojian zhou
>
>
Re: Review Request 38572: Fix CI failure for ShutdownAllDUnitTest
Posted by xiaojian zhou <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38572/
-----------------------------------------------------------
(Updated Sept. 28, 2015, 11:36 p.m.)
Review request for geode and Darrel Schneider.
Changes
-------
initialize() did set connected=true. But disconnect() set the disconnecting=true.
In IDS.isConnect(), there're following code:
if (this.isDisconnecting) {
return false;
}
return this.isConnected;
So, the "while" in following code will never enter.
- if (existingSystem.isDisconnecting()) {
- while (existingSystem.isConnected()) {
i.e. the wait(500) was never called. Anyway, even the wait(500) is never called, in old code, there's chance to create 2 instances(current thread found a disconnecting one, it goes ahead to create a new one. Then the thread2 will find 2 ds)
With our latest code change, the wait(50) will really take effect.
Bugs: geode-329
https://issues.apache.org/jira/browse/geode-329
Repository: geode
Description
-------
There's a race conditition that 2 DS will be created when the current DS is disconnecting.
Diffs (updated)
-----
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java b7b2cd8
Diff: https://reviews.apache.org/r/38572/diff/
Testing
-------
precheckin
Thanks,
xiaojian zhou
Re: Review Request 38572: Fix CI failure for ShutdownAllDUnitTest
Posted by xiaojian zhou <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38572/
-----------------------------------------------------------
(Updated Sept. 25, 2015, 11:25 p.m.)
Review request for geode and Darrel Schneider.
Changes
-------
Why the "existingSystemConnected = true;" should be put inside "while isConnected()"?
In initialize(), "disconnected" is set true
Then in disconnect(), the "disConnecting" is set to true.
Then the connect() method will not enter the "while isConnected()" loop and will not wait().
This is the root cause of the hang.
Now how to get rid of the disconnected member?
Actually addSystem(newSystem) has handled that. It added a new system into item 0, then existingSystems = Collections.unmodifiableList(l);
Bugs: geode-329
https://issues.apache.org/jira/browse/geode-329
Repository: geode
Description
-------
There's a race conditition that 2 DS will be created when the current DS is disconnecting.
Diffs (updated)
-----
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java b7b2cd8
Diff: https://reviews.apache.org/r/38572/diff/
Testing
-------
precheckin
Thanks,
xiaojian zhou
Re: Review Request 38572: Fix CI failure for ShutdownAllDUnitTest
Posted by Xiaojian Zhou <gz...@pivotal.io>.
Cannot change it to "while ..." because we will only loop when ever waited.
On Wed, Sep 23, 2015 at 5:06 PM, Darrel Schneider <ds...@pivotal.io>
wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38572/#review100324
> -----------------------------------------------------------
>
>
>
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java
> (line 1587)
> <https://reviews.apache.org/r/38572/#comment157507>
>
> Change "boolean existingSystemDisconnected = false;" to "boolean
> existingSystemDisconnected;"
> since on the very next line you initialize it.
>
>
>
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java
> (line 1619)
> <https://reviews.apache.org/r/38572/#comment157509>
>
> Change this loop from "do..while" to "while".
> Then you can get rid of the nested "if !existingSystems.isEmpty()"
> line (on line 1590).
>
>
> - Darrel Schneider
>
>
> On Sept. 23, 2015, 4:51 p.m., xiaojian zhou wrote:
> >
> > -----------------------------------------------------------
> > This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/38572/
> > -----------------------------------------------------------
> >
> > (Updated Sept. 23, 2015, 4:51 p.m.)
> >
> >
> > Review request for geode and Darrel Schneider.
> >
> >
> > Bugs: geode-329
> > https://issues.apache.org/jira/browse/geode-329
> >
> >
> > Repository: geode
> >
> >
> > Description
> > -------
> >
> > There's a race conditition that 2 DS will be created when the current DS
> is disconnecting.
> >
> >
> > Diffs
> > -----
> >
> >
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java
> b7b2cd8
> >
> > Diff: https://reviews.apache.org/r/38572/diff/
> >
> >
> > Testing
> > -------
> >
> > precheckin
> >
> >
> > Thanks,
> >
> > xiaojian zhou
> >
> >
>
>
Re: Review Request 38572: Fix CI failure for ShutdownAllDUnitTest
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38572/#review100324
-----------------------------------------------------------
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java (line 1587)
<https://reviews.apache.org/r/38572/#comment157507>
Change "boolean existingSystemDisconnected = false;" to "boolean existingSystemDisconnected;"
since on the very next line you initialize it.
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java (line 1619)
<https://reviews.apache.org/r/38572/#comment157509>
Change this loop from "do..while" to "while".
Then you can get rid of the nested "if !existingSystems.isEmpty()" line (on line 1590).
- Darrel Schneider
On Sept. 23, 2015, 4:51 p.m., xiaojian zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38572/
> -----------------------------------------------------------
>
> (Updated Sept. 23, 2015, 4:51 p.m.)
>
>
> Review request for geode and Darrel Schneider.
>
>
> Bugs: geode-329
> https://issues.apache.org/jira/browse/geode-329
>
>
> Repository: geode
>
>
> Description
> -------
>
> There's a race conditition that 2 DS will be created when the current DS is disconnecting.
>
>
> Diffs
> -----
>
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java b7b2cd8
>
> Diff: https://reviews.apache.org/r/38572/diff/
>
>
> Testing
> -------
>
> precheckin
>
>
> Thanks,
>
> xiaojian zhou
>
>
Re: Review Request 38572: Fix CI failure for ShutdownAllDUnitTest
Posted by xiaojian zhou <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38572/
-----------------------------------------------------------
(Updated Sept. 23, 2015, 11:51 p.m.)
Review request for geode and Darrel Schneider.
Changes
-------
The previous fix will hang. Fixed it.
We should only set existingSystemDisconnecte = true when wait() happened.
Bugs: geode-329
https://issues.apache.org/jira/browse/geode-329
Repository: geode
Description
-------
There's a race conditition that 2 DS will be created when the current DS is disconnecting.
Diffs (updated)
-----
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java b7b2cd8
Diff: https://reviews.apache.org/r/38572/diff/
Testing
-------
precheckin
Thanks,
xiaojian zhou