You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Sijie Guo <gu...@gmail.com> on 2012/09/14 16:06:13 UTC
Re: Review Request: Change the way remote sequence IDs are updated (BK-256)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6789/#review11536
-----------------------------------------------------------
the patch and the test looks good. I just had a minor comment and a question needs your clarification.
hedwig-protocol/src/main/java/org/apache/hedwig/protoextensions/MessageIdUtils.java
<https://reviews.apache.org/r/6789/#comment24803>
could you describe more about this case? I am not clear about it.
hedwig-server/src/test/java/org/apache/hedwig/server/integration/TestHedwigHub.java
<https://reviews.apache.org/r/6789/#comment24810>
If this changes is only used for TestHubRegion, I would prefer putting this checking in TestHubRegion. you could inherit TestMessageHandler in TestHubRegion.
- Sijie Guo
On Aug. 27, 2012, 9:27 p.m., Aniruddha Laud wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6789/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2012, 9:27 p.m.)
>
>
> Review request for bookkeeper, Ivan Kelly, Ivan Kelly, and Sijie Guo.
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/BOOKKEEPER-256
>
>
> Diffs
> -----
>
> hedwig-protocol/src/main/java/org/apache/hedwig/protoextensions/MessageIdUtils.java 9ceec26
> hedwig-server/pom.xml 9f5c3cd
> hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java d2c4298
> hedwig-server/src/main/java/org/apache/hedwig/server/persistence/LocalDBPersistenceManager.java 16b7b7d
> hedwig-server/src/main/java/org/apache/hedwig/server/persistence/ReadAheadCache.java 060a3dd
> hedwig-server/src/main/java/org/apache/hedwig/server/regions/RegionManager.java 838eca0
> hedwig-server/src/test/java/org/apache/hedwig/server/integration/TestHedwigHub.java 02b4503
> hedwig-server/src/test/java/org/apache/hedwig/server/integration/TestHedwigRegion.java 0b1851e
>
> Diff: https://reviews.apache.org/r/6789/diff/
>
>
> Testing
> -------
>
> mvn test passes.
>
>
> Thanks,
>
> Aniruddha Laud
>
>
Re: Review Request: Change the way remote sequence IDs are updated (BK-256)
Posted by Ivan Kelly <iv...@apache.org>.
> On Sept. 14, 2012, 2:06 p.m., Sijie Guo wrote:
> > hedwig-server/src/test/java/org/apache/hedwig/server/integration/TestHedwigHub.java, line 159
> > <https://reviews.apache.org/r/6789/diff/1/?file=146653#file146653line159>
> >
> > If this changes is only used for TestHubRegion, I would prefer putting this checking in TestHubRegion. you could inherit TestMessageHandler in TestHubRegion.
It represents an invariant that should hold all the time though, so I don't think there's a problem keeping it here. It may flag bad changes in the future.
> On Sept. 14, 2012, 2:06 p.m., Sijie Guo wrote:
> > hedwig-protocol/src/main/java/org/apache/hedwig/protoextensions/MessageIdUtils.java, line 143
> > <https://reviews.apache.org/r/6789/diff/1/?file=146647#file146647line143>
> >
> > could you describe more about this case? I am not clear about it.
I'm also unclear on this. In this else branch, you're basically copying all region specific seqid which are not from the src region. This is correct. I don't see where redelivery comes into it.
- Ivan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6789/#review11536
-----------------------------------------------------------
On Aug. 27, 2012, 9:27 p.m., Aniruddha Laud wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6789/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2012, 9:27 p.m.)
>
>
> Review request for bookkeeper, Ivan Kelly, Ivan Kelly, and Sijie Guo.
>
>
> Description
> -------
>
> https://issues.apache.org/jira/browse/BOOKKEEPER-256
>
>
> Diffs
> -----
>
> hedwig-protocol/src/main/java/org/apache/hedwig/protoextensions/MessageIdUtils.java 9ceec26
> hedwig-server/pom.xml 9f5c3cd
> hedwig-server/src/main/java/org/apache/hedwig/server/persistence/BookkeeperPersistenceManager.java d2c4298
> hedwig-server/src/main/java/org/apache/hedwig/server/persistence/LocalDBPersistenceManager.java 16b7b7d
> hedwig-server/src/main/java/org/apache/hedwig/server/persistence/ReadAheadCache.java 060a3dd
> hedwig-server/src/main/java/org/apache/hedwig/server/regions/RegionManager.java 838eca0
> hedwig-server/src/test/java/org/apache/hedwig/server/integration/TestHedwigHub.java 02b4503
> hedwig-server/src/test/java/org/apache/hedwig/server/integration/TestHedwigRegion.java 0b1851e
>
> Diff: https://reviews.apache.org/r/6789/diff/
>
>
> Testing
> -------
>
> mvn test passes.
>
>
> Thanks,
>
> Aniruddha Laud
>
>