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/17 14:13:28 UTC
Review Request: BOOKKEEPER-364: re-factor hedwig java client to support both
one-subscription-per-channel and multiplex-subscriptions-per-channel.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7129/
-----------------------------------------------------------
Review request for bookkeeper.
Description
-------
Currently Hedwig client is coupled with the design of one subscription per channel. In order to multiplex multiple subscription into one channel, it would be better to refine the Hedwig java client code to provide better interface to support both mode.
This addresses bug BOOKKEEPER-364.
https://issues.apache.org/jira/browse/BOOKKEEPER-364
Diffs
-----
hedwig-client/src/main/java/org/apache/hedwig/client/data/MessageConsumeData.java ccb9903
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/AbstractResponseHandler.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/MessageConsumeCallback.java d8f9324
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/PublishResponseHandler.java d83078e
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/SubscribeReconnectCallback.java f746803
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/SubscribeResponseHandler.java ac209d1
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/UnsubscribeResponseHandler.java f77417f
hedwig-client/src/main/java/org/apache/hedwig/client/netty/AbstractHChannelManager.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/CleanupChannelMap.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/ClientChannelPipelineFactory.java ca6d706
hedwig-client/src/main/java/org/apache/hedwig/client/netty/ConnectCallback.java 68867a3
hedwig-client/src/main/java/org/apache/hedwig/client/netty/DefaultServerChannel.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/FilterableMessageHandler.java a97bab9
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannel.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannelHandler.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannelImpl.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannelManager.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigClientImpl.java 39d9a86
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigPublisher.java 0891945
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigSubscriber.java 81af1dd
hedwig-client/src/main/java/org/apache/hedwig/client/netty/NetUtils.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/NonSubscriptionChannelPipelineFactory.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/ResponseHandler.java 127cfb8
hedwig-client/src/main/java/org/apache/hedwig/client/netty/SubscriptionEventEmitter.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/WriteCallback.java 41a19d2
hedwig-client/src/main/java/org/apache/hedwig/client/netty/package-info.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/simple/SimpleHChannelManager.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/simple/SimpleSubscribeResponseHandler.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/simple/SimpleSubscriptionChannelPipelineFactory.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/simple/SubscribeReconnectCallback.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/util/VarArgs.java PRE-CREATION
Diff: https://reviews.apache.org/r/7129/diff/
Testing
-------
Thanks,
Sijie Guo
Re: Review Request: BOOKKEEPER-364: re-factor hedwig java client to support
both one-subscription-per-channel and multiplex-subscriptions-per-channel.
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7129/
-----------------------------------------------------------
(Updated Sept. 23, 2012, 12:28 p.m.)
Review request for bookkeeper.
Changes
-------
addressing Ivan' comments to move abstract things to client.netty.impl package.
Description
-------
Currently Hedwig client is coupled with the design of one subscription per channel. In order to multiplex multiple subscription into one channel, it would be better to refine the Hedwig java client code to provide better interface to support both mode.
This addresses bug BOOKKEEPER-364.
https://issues.apache.org/jira/browse/BOOKKEEPER-364
Diffs (updated)
-----
hedwig-client/src/main/java/org/apache/hedwig/client/data/MessageConsumeData.java ccb9903
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/AbstractResponseHandler.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/MessageConsumeCallback.java d8f9324
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/PublishResponseHandler.java d83078e
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/SubscribeReconnectCallback.java f746803
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/SubscribeResponseHandler.java ac209d1
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/UnsubscribeResponseHandler.java f77417f
hedwig-client/src/main/java/org/apache/hedwig/client/netty/CleanupChannelMap.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/ClientChannelPipelineFactory.java ca6d706
hedwig-client/src/main/java/org/apache/hedwig/client/netty/ConnectCallback.java 68867a3
hedwig-client/src/main/java/org/apache/hedwig/client/netty/FilterableMessageHandler.java a97bab9
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannel.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannelManager.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigClientImpl.java 39d9a86
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigPublisher.java 0891945
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigSubscriber.java 81af1dd
hedwig-client/src/main/java/org/apache/hedwig/client/netty/NetUtils.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/ResponseHandler.java 127cfb8
hedwig-client/src/main/java/org/apache/hedwig/client/netty/SubscriptionEventEmitter.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/WriteCallback.java 41a19d2
hedwig-client/src/main/java/org/apache/hedwig/client/netty/impl/AbstractHChannelManager.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/impl/ClientChannelPipelineFactory.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/impl/DefaultServerChannel.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/impl/HChannelHandler.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/impl/HChannelImpl.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/impl/NonSubscriptionChannelPipelineFactory.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/impl/WriteCallback.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/impl/simple/SimpleHChannelManager.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/impl/simple/SimpleSubscribeResponseHandler.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/impl/simple/SimpleSubscriptionChannelPipelineFactory.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/impl/simple/SubscribeReconnectCallback.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/package-info.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/util/VarArgs.java PRE-CREATION
hedwig-server/src/test/java/org/apache/hedwig/client/TestPubSubClient.java aba0044
Diff: https://reviews.apache.org/r/7129/diff/
Testing
-------
Thanks,
Sijie Guo
Re: Review Request: BOOKKEEPER-364: re-factor hedwig java client to support
both one-subscription-per-channel and multiplex-subscriptions-per-channel.
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7129/
-----------------------------------------------------------
(Updated Sept. 20, 2012, 3:54 p.m.)
Review request for bookkeeper.
Changes
-------
Update a new patch addressing Ivan's comments.
Description
-------
Currently Hedwig client is coupled with the design of one subscription per channel. In order to multiplex multiple subscription into one channel, it would be better to refine the Hedwig java client code to provide better interface to support both mode.
This addresses bug BOOKKEEPER-364.
https://issues.apache.org/jira/browse/BOOKKEEPER-364
Diffs (updated)
-----
hedwig-client/src/main/java/org/apache/hedwig/client/data/MessageConsumeData.java ccb9903
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/AbstractResponseHandler.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/MessageConsumeCallback.java d8f9324
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/PublishResponseHandler.java d83078e
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/SubscribeReconnectCallback.java f746803
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/SubscribeResponseHandler.java ac209d1
hedwig-client/src/main/java/org/apache/hedwig/client/handlers/UnsubscribeResponseHandler.java f77417f
hedwig-client/src/main/java/org/apache/hedwig/client/netty/AbstractHChannelManager.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/CleanupChannelMap.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/ClientChannelPipelineFactory.java ca6d706
hedwig-client/src/main/java/org/apache/hedwig/client/netty/ConnectCallback.java 68867a3
hedwig-client/src/main/java/org/apache/hedwig/client/netty/DefaultServerChannel.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/FilterableMessageHandler.java a97bab9
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannel.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannelHandler.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannelImpl.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannelManager.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigClientImpl.java 39d9a86
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigPublisher.java 0891945
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigSubscriber.java 81af1dd
hedwig-client/src/main/java/org/apache/hedwig/client/netty/NetUtils.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/NonSubscriptionChannelPipelineFactory.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/ResponseHandler.java 127cfb8
hedwig-client/src/main/java/org/apache/hedwig/client/netty/SubscriptionEventEmitter.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/WriteCallback.java 41a19d2
hedwig-client/src/main/java/org/apache/hedwig/client/netty/package-info.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/simple/SimpleHChannelManager.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/simple/SimpleSubscribeResponseHandler.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/simple/SimpleSubscriptionChannelPipelineFactory.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/client/netty/simple/SubscribeReconnectCallback.java PRE-CREATION
hedwig-client/src/main/java/org/apache/hedwig/util/VarArgs.java PRE-CREATION
Diff: https://reviews.apache.org/r/7129/diff/
Testing
-------
Thanks,
Sijie Guo
Re: Review Request: BOOKKEEPER-364: re-factor hedwig java client to support
both one-subscription-per-channel and multiplex-subscriptions-per-channel.
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7129/#review11689
-----------------------------------------------------------
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannelManager.java
<https://reviews.apache.org/r/7129/#comment25268>
rename to submitOpAfterDelay
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannelManager.java
<https://reviews.apache.org/r/7129/#comment25266>
rename to redirectToHost
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannelManager.java
<https://reviews.apache.org/r/7129/#comment25265>
This shouldn't be part of this interface. It should be part of AbstractHChannelManager as it's only ever called from DefaultServerChannel.
hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigClientImpl.java
<https://reviews.apache.org/r/7129/#comment25267>
You're breaking your interface here by having getHChannelManager return an AbstractHChannelManager rather than just a HChannelManager. It means the HChannelManager interface is redundant because every implementation would have to extend AbstractHChannelManager to be useful. In effect, AbstractHChannelManager is now the interface.
The simple fix is to move the getSubscribeResponseHandler, startDelivery & stopDelivery, asyncCloseSubscription, getSubscriptionEventEmitter from AbstractHChannelManager to the interface.
This isn't nice though, because it conflates the concepts of channel management and subscription management, but it's better to have a messy interface, than one that you are bypassing all the time.
- Ivan Kelly
On Sept. 17, 2012, 12:13 p.m., Sijie Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7129/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2012, 12:13 p.m.)
>
>
> Review request for bookkeeper.
>
>
> Description
> -------
>
> Currently Hedwig client is coupled with the design of one subscription per channel. In order to multiplex multiple subscription into one channel, it would be better to refine the Hedwig java client code to provide better interface to support both mode.
>
>
> This addresses bug BOOKKEEPER-364.
> https://issues.apache.org/jira/browse/BOOKKEEPER-364
>
>
> Diffs
> -----
>
> hedwig-client/src/main/java/org/apache/hedwig/client/data/MessageConsumeData.java ccb9903
> hedwig-client/src/main/java/org/apache/hedwig/client/handlers/AbstractResponseHandler.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/client/handlers/MessageConsumeCallback.java d8f9324
> hedwig-client/src/main/java/org/apache/hedwig/client/handlers/PublishResponseHandler.java d83078e
> hedwig-client/src/main/java/org/apache/hedwig/client/handlers/SubscribeReconnectCallback.java f746803
> hedwig-client/src/main/java/org/apache/hedwig/client/handlers/SubscribeResponseHandler.java ac209d1
> hedwig-client/src/main/java/org/apache/hedwig/client/handlers/UnsubscribeResponseHandler.java f77417f
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/AbstractHChannelManager.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/CleanupChannelMap.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/ClientChannelPipelineFactory.java ca6d706
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/ConnectCallback.java 68867a3
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/DefaultServerChannel.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/FilterableMessageHandler.java a97bab9
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannel.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannelHandler.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannelImpl.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/HChannelManager.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigClientImpl.java 39d9a86
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigPublisher.java 0891945
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/HedwigSubscriber.java 81af1dd
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/NetUtils.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/NonSubscriptionChannelPipelineFactory.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/ResponseHandler.java 127cfb8
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/SubscriptionEventEmitter.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/WriteCallback.java 41a19d2
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/package-info.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/simple/SimpleHChannelManager.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/simple/SimpleSubscribeResponseHandler.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/simple/SimpleSubscriptionChannelPipelineFactory.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/client/netty/simple/SubscribeReconnectCallback.java PRE-CREATION
> hedwig-client/src/main/java/org/apache/hedwig/util/VarArgs.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/7129/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Sijie Guo
>
>