You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Ivan Kelly <iv...@yahoo-inc.com> on 2011/10/19 17:40:31 UTC

Review of public APIs

I've just gone through the public apis for BK and HW and have found the following issues. Most of them are issues with protection being too loose. There's only 2-3 breaks here which should require code change on the users' part. The rest are things that they really shouldn't be using in any case.

BookKeeper#createLedger, parameter is named passwd, "Key" used in LedgerHandle api
BookKeeper#getBookieClient shouldn't be public
BookKeeper#createComplete shouldn't be public
BookKeeper#openComplete shouldn't be public
BookKeeper#deleteComplete shouldn't be public
BookKeeper#halt could be changed to close(), should throw a BKException

LedgerHandle#getLedgerKey passwd is used in BookKeeper, should possibly be private
LedgerHandle#getLedgerMetadata shouldn't be public
LedgerHandle#getDigestManager shouldn't be public
LedgerHandle#getDistributionSchedule shouldn't be public
LedgerHandle#writeLedgerConfig shouldn't be public
LedgerHandle#addEntry should return void, errors should go in an Exception
LedgerHandle#readComplete should not be public
LedgerHandle#addComplete should not be public
LedgerHandle#readLastConfirmedCompelte should not be public
LedgerHandle#closeComplete should not be public

ASyncCallback#RecoverCallback shouldn't be public

HedwigClient#getSslFactory shouldn't be public
HedwigClient#getConsumeCallback shouldn't be public
HedwigClient#doConnect shouldn't be public
HedwigClient#getHostFromChannel shouldn't be public
HedwigClient#getResponseHandlerFromChannel shouldn't be public
HedwigClient#getHostForTopic shouldn't be public
HedwigClient#clearAllTopicsForHost shouldn't be public
HedwigClient#getClientTimer shoulnd't be public
HedwigClient#stop should throw some sort of Exception in the case of errors

HedwigPublisher#publish shouldn't use protobuf ByteString, as it requires the user to import protobufs
HedwigPublisher#getChannelForHost shouldn't be public

HedwigSubscriber#HedwigSubscriber shouldn't be public
HedwigSubscriber#doConsume shouldn't be public
HedwigSubscriber#hasSubscription probably shouldn't be public
HedwigSubscriber#getSubscriptionList shoulnd't exist
HedwigSubscriber#getChannelForTopic shouldn't be public
HedwigSubscriber#setChannelforTopic shouldn't be public
HedwigSubscriber#removeChannelForTopic shound't be public

MessageHandler#consume should be called 'deliver'

The hedwig client is under a netty package. There's nothing netty specific about the api, so it should be in the org.apache.hedwig.client package. 

In both BK and HW, context objects are passed around with callbacks. I don't think this is necessary, but it doesn't bug me too much. 


-Ivan

Re: Review of public APIs

Posted by Ivan Kelly <iv...@yahoo-inc.com>.
> For BOOKKEEPER-90, I still need to decide what to do about ByteString if we do anything at all. Having ByteString in the API tries us to google protobufs. Im not sure this is a huge issue, but it means that this API will never be able to move away from it.

Another open issue is that HedwigClient#stop doesn't throw any exception at the moment. Should we add an exception? Also, I think it should be renamed to close().

Btw, "mvn compile site" will generate javadoc in target/site/apidocs/index.html

-Ivan

Re: Review of public APIs

Posted by Ivan Kelly <iv...@yahoo-inc.com>.
Yes, I agree on the 4.0 release thing. It will make it clear that there's been a big change.

I've created two JIRAs for the changes, BOOKKEEPER-89 for bookkeeper, BOOKKEEPER-90 for hedwig. Please have a look [1][2].

BOOKKEEPER-89 is complete.

For BOOKKEEPER-90, I still need to decide what to do about ByteString if we do anything at all. Having ByteString in the API tries us to google protobufs. Im not sure this is a huge issue, but it means that this API will never be able to move away from it.

-Ivan

[1] https://reviews.apache.org/r/2543/
[2] https://reviews.apache.org/r/2546/

On 22 Oct 2011, at 00:27, Benjamin Reed wrote:

> i also agee. we should also call it a 4.0 release.
> 
> ben
> 
> On Fri, Oct 21, 2011 at 2:17 AM, Ivan Kelly <iv...@yahoo-inc.com> wrote:
>> I agree with Utkarsh. Initial release is as good of an excuse we'll ever have for breaking stuff. The changes themselves shouldn't be too big though. Ill have a go later today.
>> 
>> -Ivan
>> 
>> On 19 Oct 2011, at 19:04, Utkarsh Srivastava wrote:
>> 
>>> I think it makes sense to delay the release if required for this
>>> cleanup. Such cleanup will get only harder after the release.
>>> 
>>> On Wed, Oct 19, 2011 at 9:42 AM, Flavio Junqueira <fp...@yahoo-inc.com> wrote:
>>>> Hi Ivan, Thanks for putting this list together. I don't have a good sense of
>>>> how many changes the modifications you're proposing would induce. My concern
>>>> is delaying the release further, even though I agree that cleaning up the
>>>> interfaces is important.
>>>> 
>>>> Also, I was thinking that some public calls are related to our package
>>>> structure. In particular, I'm referring to BookKeeperTools, which is in this
>>>> tools package and I've needed to make some calls public to be able to access
>>>> from there. It might be a good idea to review those as well.
>>>> 
>>>> -Flavio
>>>> 
>>>> On Oct 19, 2011, at 5:40 PM, Ivan Kelly wrote:
>>>> 
>>>>> I've just gone through the public apis for BK and HW and have found the
>>>>> following issues. Most of them are issues with protection being too loose.
>>>>> There's only 2-3 breaks here which should require code change on the users'
>>>>> part. The rest are things that they really shouldn't be using in any case.
>>>>> 
>>>>> BookKeeper#createLedger, parameter is named passwd, "Key" used in
>>>>> LedgerHandle api
>>>>> BookKeeper#getBookieClient shouldn't be public
>>>>> BookKeeper#createComplete shouldn't be public
>>>>> BookKeeper#openComplete shouldn't be public
>>>>> BookKeeper#deleteComplete shouldn't be public
>>>>> BookKeeper#halt could be changed to close(), should throw a BKException
>>>>> 
>>>>> LedgerHandle#getLedgerKey passwd is used in BookKeeper, should possibly be
>>>>> private
>>>>> LedgerHandle#getLedgerMetadata shouldn't be public
>>>>> LedgerHandle#getDigestManager shouldn't be public
>>>>> LedgerHandle#getDistributionSchedule shouldn't be public
>>>>> LedgerHandle#writeLedgerConfig shouldn't be public
>>>>> LedgerHandle#addEntry should return void, errors should go in an Exception
>>>>> LedgerHandle#readComplete should not be public
>>>>> LedgerHandle#addComplete should not be public
>>>>> LedgerHandle#readLastConfirmedCompelte should not be public
>>>>> LedgerHandle#closeComplete should not be public
>>>>> 
>>>>> ASyncCallback#RecoverCallback shouldn't be public
>>>>> 
>>>>> HedwigClient#getSslFactory shouldn't be public
>>>>> HedwigClient#getConsumeCallback shouldn't be public
>>>>> HedwigClient#doConnect shouldn't be public
>>>>> HedwigClient#getHostFromChannel shouldn't be public
>>>>> HedwigClient#getResponseHandlerFromChannel shouldn't be public
>>>>> HedwigClient#getHostForTopic shouldn't be public
>>>>> HedwigClient#clearAllTopicsForHost shouldn't be public
>>>>> HedwigClient#getClientTimer shoulnd't be public
>>>>> HedwigClient#stop should throw some sort of Exception in the case of
>>>>> errors
>>>>> 
>>>>> HedwigPublisher#publish shouldn't use protobuf ByteString, as it requires
>>>>> the user to import protobufs
>>>>> HedwigPublisher#getChannelForHost shouldn't be public
>>>>> 
>>>>> HedwigSubscriber#HedwigSubscriber shouldn't be public
>>>>> HedwigSubscriber#doConsume shouldn't be public
>>>>> HedwigSubscriber#hasSubscription probably shouldn't be public
>>>>> HedwigSubscriber#getSubscriptionList shoulnd't exist
>>>>> HedwigSubscriber#getChannelForTopic shouldn't be public
>>>>> HedwigSubscriber#setChannelforTopic shouldn't be public
>>>>> HedwigSubscriber#removeChannelForTopic shound't be public
>>>>> 
>>>>> MessageHandler#consume should be called 'deliver'
>>>>> 
>>>>> The hedwig client is under a netty package. There's nothing netty specific
>>>>> about the api, so it should be in the org.apache.hedwig.client package.
>>>>> 
>>>>> In both BK and HW, context objects are passed around with callbacks. I
>>>>> don't think this is necessary, but it doesn't bug me too much.
>>>>> 
>>>>> 
>>>>> -Ivan
>>>> 
>>>> flavio
>>>> junqueira
>>>> 
>>>> research scientist
>>>> 
>>>> fpj@yahoo-inc.com
>>>> direct +34 93-183-8828
>>>> 
>>>> avinguda diagonal 177, 8th floor, barcelona, 08018, es
>>>> phone (408) 349 3300    fax (408) 349 3301
>>>> 
>>>> 
>> 
>> 


Re: Review of public APIs

Posted by Benjamin Reed <br...@apache.org>.
i also agee. we should also call it a 4.0 release.

ben

On Fri, Oct 21, 2011 at 2:17 AM, Ivan Kelly <iv...@yahoo-inc.com> wrote:
> I agree with Utkarsh. Initial release is as good of an excuse we'll ever have for breaking stuff. The changes themselves shouldn't be too big though. Ill have a go later today.
>
> -Ivan
>
> On 19 Oct 2011, at 19:04, Utkarsh Srivastava wrote:
>
>> I think it makes sense to delay the release if required for this
>> cleanup. Such cleanup will get only harder after the release.
>>
>> On Wed, Oct 19, 2011 at 9:42 AM, Flavio Junqueira <fp...@yahoo-inc.com> wrote:
>>> Hi Ivan, Thanks for putting this list together. I don't have a good sense of
>>> how many changes the modifications you're proposing would induce. My concern
>>> is delaying the release further, even though I agree that cleaning up the
>>> interfaces is important.
>>>
>>> Also, I was thinking that some public calls are related to our package
>>> structure. In particular, I'm referring to BookKeeperTools, which is in this
>>> tools package and I've needed to make some calls public to be able to access
>>> from there. It might be a good idea to review those as well.
>>>
>>> -Flavio
>>>
>>> On Oct 19, 2011, at 5:40 PM, Ivan Kelly wrote:
>>>
>>>> I've just gone through the public apis for BK and HW and have found the
>>>> following issues. Most of them are issues with protection being too loose.
>>>> There's only 2-3 breaks here which should require code change on the users'
>>>> part. The rest are things that they really shouldn't be using in any case.
>>>>
>>>> BookKeeper#createLedger, parameter is named passwd, "Key" used in
>>>> LedgerHandle api
>>>> BookKeeper#getBookieClient shouldn't be public
>>>> BookKeeper#createComplete shouldn't be public
>>>> BookKeeper#openComplete shouldn't be public
>>>> BookKeeper#deleteComplete shouldn't be public
>>>> BookKeeper#halt could be changed to close(), should throw a BKException
>>>>
>>>> LedgerHandle#getLedgerKey passwd is used in BookKeeper, should possibly be
>>>> private
>>>> LedgerHandle#getLedgerMetadata shouldn't be public
>>>> LedgerHandle#getDigestManager shouldn't be public
>>>> LedgerHandle#getDistributionSchedule shouldn't be public
>>>> LedgerHandle#writeLedgerConfig shouldn't be public
>>>> LedgerHandle#addEntry should return void, errors should go in an Exception
>>>> LedgerHandle#readComplete should not be public
>>>> LedgerHandle#addComplete should not be public
>>>> LedgerHandle#readLastConfirmedCompelte should not be public
>>>> LedgerHandle#closeComplete should not be public
>>>>
>>>> ASyncCallback#RecoverCallback shouldn't be public
>>>>
>>>> HedwigClient#getSslFactory shouldn't be public
>>>> HedwigClient#getConsumeCallback shouldn't be public
>>>> HedwigClient#doConnect shouldn't be public
>>>> HedwigClient#getHostFromChannel shouldn't be public
>>>> HedwigClient#getResponseHandlerFromChannel shouldn't be public
>>>> HedwigClient#getHostForTopic shouldn't be public
>>>> HedwigClient#clearAllTopicsForHost shouldn't be public
>>>> HedwigClient#getClientTimer shoulnd't be public
>>>> HedwigClient#stop should throw some sort of Exception in the case of
>>>> errors
>>>>
>>>> HedwigPublisher#publish shouldn't use protobuf ByteString, as it requires
>>>> the user to import protobufs
>>>> HedwigPublisher#getChannelForHost shouldn't be public
>>>>
>>>> HedwigSubscriber#HedwigSubscriber shouldn't be public
>>>> HedwigSubscriber#doConsume shouldn't be public
>>>> HedwigSubscriber#hasSubscription probably shouldn't be public
>>>> HedwigSubscriber#getSubscriptionList shoulnd't exist
>>>> HedwigSubscriber#getChannelForTopic shouldn't be public
>>>> HedwigSubscriber#setChannelforTopic shouldn't be public
>>>> HedwigSubscriber#removeChannelForTopic shound't be public
>>>>
>>>> MessageHandler#consume should be called 'deliver'
>>>>
>>>> The hedwig client is under a netty package. There's nothing netty specific
>>>> about the api, so it should be in the org.apache.hedwig.client package.
>>>>
>>>> In both BK and HW, context objects are passed around with callbacks. I
>>>> don't think this is necessary, but it doesn't bug me too much.
>>>>
>>>>
>>>> -Ivan
>>>
>>> flavio
>>> junqueira
>>>
>>> research scientist
>>>
>>> fpj@yahoo-inc.com
>>> direct +34 93-183-8828
>>>
>>> avinguda diagonal 177, 8th floor, barcelona, 08018, es
>>> phone (408) 349 3300    fax (408) 349 3301
>>>
>>>
>
>

Re: Review of public APIs

Posted by Ivan Kelly <iv...@yahoo-inc.com>.
I agree with Utkarsh. Initial release is as good of an excuse we'll ever have for breaking stuff. The changes themselves shouldn't be too big though. Ill have a go later today.

-Ivan

On 19 Oct 2011, at 19:04, Utkarsh Srivastava wrote:

> I think it makes sense to delay the release if required for this
> cleanup. Such cleanup will get only harder after the release.
> 
> On Wed, Oct 19, 2011 at 9:42 AM, Flavio Junqueira <fp...@yahoo-inc.com> wrote:
>> Hi Ivan, Thanks for putting this list together. I don't have a good sense of
>> how many changes the modifications you're proposing would induce. My concern
>> is delaying the release further, even though I agree that cleaning up the
>> interfaces is important.
>> 
>> Also, I was thinking that some public calls are related to our package
>> structure. In particular, I'm referring to BookKeeperTools, which is in this
>> tools package and I've needed to make some calls public to be able to access
>> from there. It might be a good idea to review those as well.
>> 
>> -Flavio
>> 
>> On Oct 19, 2011, at 5:40 PM, Ivan Kelly wrote:
>> 
>>> I've just gone through the public apis for BK and HW and have found the
>>> following issues. Most of them are issues with protection being too loose.
>>> There's only 2-3 breaks here which should require code change on the users'
>>> part. The rest are things that they really shouldn't be using in any case.
>>> 
>>> BookKeeper#createLedger, parameter is named passwd, "Key" used in
>>> LedgerHandle api
>>> BookKeeper#getBookieClient shouldn't be public
>>> BookKeeper#createComplete shouldn't be public
>>> BookKeeper#openComplete shouldn't be public
>>> BookKeeper#deleteComplete shouldn't be public
>>> BookKeeper#halt could be changed to close(), should throw a BKException
>>> 
>>> LedgerHandle#getLedgerKey passwd is used in BookKeeper, should possibly be
>>> private
>>> LedgerHandle#getLedgerMetadata shouldn't be public
>>> LedgerHandle#getDigestManager shouldn't be public
>>> LedgerHandle#getDistributionSchedule shouldn't be public
>>> LedgerHandle#writeLedgerConfig shouldn't be public
>>> LedgerHandle#addEntry should return void, errors should go in an Exception
>>> LedgerHandle#readComplete should not be public
>>> LedgerHandle#addComplete should not be public
>>> LedgerHandle#readLastConfirmedCompelte should not be public
>>> LedgerHandle#closeComplete should not be public
>>> 
>>> ASyncCallback#RecoverCallback shouldn't be public
>>> 
>>> HedwigClient#getSslFactory shouldn't be public
>>> HedwigClient#getConsumeCallback shouldn't be public
>>> HedwigClient#doConnect shouldn't be public
>>> HedwigClient#getHostFromChannel shouldn't be public
>>> HedwigClient#getResponseHandlerFromChannel shouldn't be public
>>> HedwigClient#getHostForTopic shouldn't be public
>>> HedwigClient#clearAllTopicsForHost shouldn't be public
>>> HedwigClient#getClientTimer shoulnd't be public
>>> HedwigClient#stop should throw some sort of Exception in the case of
>>> errors
>>> 
>>> HedwigPublisher#publish shouldn't use protobuf ByteString, as it requires
>>> the user to import protobufs
>>> HedwigPublisher#getChannelForHost shouldn't be public
>>> 
>>> HedwigSubscriber#HedwigSubscriber shouldn't be public
>>> HedwigSubscriber#doConsume shouldn't be public
>>> HedwigSubscriber#hasSubscription probably shouldn't be public
>>> HedwigSubscriber#getSubscriptionList shoulnd't exist
>>> HedwigSubscriber#getChannelForTopic shouldn't be public
>>> HedwigSubscriber#setChannelforTopic shouldn't be public
>>> HedwigSubscriber#removeChannelForTopic shound't be public
>>> 
>>> MessageHandler#consume should be called 'deliver'
>>> 
>>> The hedwig client is under a netty package. There's nothing netty specific
>>> about the api, so it should be in the org.apache.hedwig.client package.
>>> 
>>> In both BK and HW, context objects are passed around with callbacks. I
>>> don't think this is necessary, but it doesn't bug me too much.
>>> 
>>> 
>>> -Ivan
>> 
>> flavio
>> junqueira
>> 
>> research scientist
>> 
>> fpj@yahoo-inc.com
>> direct +34 93-183-8828
>> 
>> avinguda diagonal 177, 8th floor, barcelona, 08018, es
>> phone (408) 349 3300    fax (408) 349 3301
>> 
>> 


Re: Review of public APIs

Posted by Utkarsh Srivastava <ut...@twitter.com>.
I think it makes sense to delay the release if required for this
cleanup. Such cleanup will get only harder after the release.

On Wed, Oct 19, 2011 at 9:42 AM, Flavio Junqueira <fp...@yahoo-inc.com> wrote:
> Hi Ivan, Thanks for putting this list together. I don't have a good sense of
> how many changes the modifications you're proposing would induce. My concern
> is delaying the release further, even though I agree that cleaning up the
> interfaces is important.
>
> Also, I was thinking that some public calls are related to our package
> structure. In particular, I'm referring to BookKeeperTools, which is in this
> tools package and I've needed to make some calls public to be able to access
> from there. It might be a good idea to review those as well.
>
> -Flavio
>
> On Oct 19, 2011, at 5:40 PM, Ivan Kelly wrote:
>
>> I've just gone through the public apis for BK and HW and have found the
>> following issues. Most of them are issues with protection being too loose.
>> There's only 2-3 breaks here which should require code change on the users'
>> part. The rest are things that they really shouldn't be using in any case.
>>
>> BookKeeper#createLedger, parameter is named passwd, "Key" used in
>> LedgerHandle api
>> BookKeeper#getBookieClient shouldn't be public
>> BookKeeper#createComplete shouldn't be public
>> BookKeeper#openComplete shouldn't be public
>> BookKeeper#deleteComplete shouldn't be public
>> BookKeeper#halt could be changed to close(), should throw a BKException
>>
>> LedgerHandle#getLedgerKey passwd is used in BookKeeper, should possibly be
>> private
>> LedgerHandle#getLedgerMetadata shouldn't be public
>> LedgerHandle#getDigestManager shouldn't be public
>> LedgerHandle#getDistributionSchedule shouldn't be public
>> LedgerHandle#writeLedgerConfig shouldn't be public
>> LedgerHandle#addEntry should return void, errors should go in an Exception
>> LedgerHandle#readComplete should not be public
>> LedgerHandle#addComplete should not be public
>> LedgerHandle#readLastConfirmedCompelte should not be public
>> LedgerHandle#closeComplete should not be public
>>
>> ASyncCallback#RecoverCallback shouldn't be public
>>
>> HedwigClient#getSslFactory shouldn't be public
>> HedwigClient#getConsumeCallback shouldn't be public
>> HedwigClient#doConnect shouldn't be public
>> HedwigClient#getHostFromChannel shouldn't be public
>> HedwigClient#getResponseHandlerFromChannel shouldn't be public
>> HedwigClient#getHostForTopic shouldn't be public
>> HedwigClient#clearAllTopicsForHost shouldn't be public
>> HedwigClient#getClientTimer shoulnd't be public
>> HedwigClient#stop should throw some sort of Exception in the case of
>> errors
>>
>> HedwigPublisher#publish shouldn't use protobuf ByteString, as it requires
>> the user to import protobufs
>> HedwigPublisher#getChannelForHost shouldn't be public
>>
>> HedwigSubscriber#HedwigSubscriber shouldn't be public
>> HedwigSubscriber#doConsume shouldn't be public
>> HedwigSubscriber#hasSubscription probably shouldn't be public
>> HedwigSubscriber#getSubscriptionList shoulnd't exist
>> HedwigSubscriber#getChannelForTopic shouldn't be public
>> HedwigSubscriber#setChannelforTopic shouldn't be public
>> HedwigSubscriber#removeChannelForTopic shound't be public
>>
>> MessageHandler#consume should be called 'deliver'
>>
>> The hedwig client is under a netty package. There's nothing netty specific
>> about the api, so it should be in the org.apache.hedwig.client package.
>>
>> In both BK and HW, context objects are passed around with callbacks. I
>> don't think this is necessary, but it doesn't bug me too much.
>>
>>
>> -Ivan
>
> flavio
> junqueira
>
> research scientist
>
> fpj@yahoo-inc.com
> direct +34 93-183-8828
>
> avinguda diagonal 177, 8th floor, barcelona, 08018, es
> phone (408) 349 3300    fax (408) 349 3301
>
>

Re: Review of public APIs

Posted by Flavio Junqueira <fp...@yahoo-inc.com>.
Hi Ivan, Thanks for putting this list together. I don't have a good  
sense of how many changes the modifications you're proposing would  
induce. My concern is delaying the release further, even though I  
agree that cleaning up the interfaces is important.

Also, I was thinking that some public calls are related to our package  
structure. In particular, I'm referring to BookKeeperTools, which is  
in this tools package and I've needed to make some calls public to be  
able to access from there. It might be a good idea to review those as  
well.

-Flavio

On Oct 19, 2011, at 5:40 PM, Ivan Kelly wrote:

> I've just gone through the public apis for BK and HW and have found  
> the following issues. Most of them are issues with protection being  
> too loose. There's only 2-3 breaks here which should require code  
> change on the users' part. The rest are things that they really  
> shouldn't be using in any case.
>
> BookKeeper#createLedger, parameter is named passwd, "Key" used in  
> LedgerHandle api
> BookKeeper#getBookieClient shouldn't be public
> BookKeeper#createComplete shouldn't be public
> BookKeeper#openComplete shouldn't be public
> BookKeeper#deleteComplete shouldn't be public
> BookKeeper#halt could be changed to close(), should throw a  
> BKException
>
> LedgerHandle#getLedgerKey passwd is used in BookKeeper, should  
> possibly be private
> LedgerHandle#getLedgerMetadata shouldn't be public
> LedgerHandle#getDigestManager shouldn't be public
> LedgerHandle#getDistributionSchedule shouldn't be public
> LedgerHandle#writeLedgerConfig shouldn't be public
> LedgerHandle#addEntry should return void, errors should go in an  
> Exception
> LedgerHandle#readComplete should not be public
> LedgerHandle#addComplete should not be public
> LedgerHandle#readLastConfirmedCompelte should not be public
> LedgerHandle#closeComplete should not be public
>
> ASyncCallback#RecoverCallback shouldn't be public
>
> HedwigClient#getSslFactory shouldn't be public
> HedwigClient#getConsumeCallback shouldn't be public
> HedwigClient#doConnect shouldn't be public
> HedwigClient#getHostFromChannel shouldn't be public
> HedwigClient#getResponseHandlerFromChannel shouldn't be public
> HedwigClient#getHostForTopic shouldn't be public
> HedwigClient#clearAllTopicsForHost shouldn't be public
> HedwigClient#getClientTimer shoulnd't be public
> HedwigClient#stop should throw some sort of Exception in the case of  
> errors
>
> HedwigPublisher#publish shouldn't use protobuf ByteString, as it  
> requires the user to import protobufs
> HedwigPublisher#getChannelForHost shouldn't be public
>
> HedwigSubscriber#HedwigSubscriber shouldn't be public
> HedwigSubscriber#doConsume shouldn't be public
> HedwigSubscriber#hasSubscription probably shouldn't be public
> HedwigSubscriber#getSubscriptionList shoulnd't exist
> HedwigSubscriber#getChannelForTopic shouldn't be public
> HedwigSubscriber#setChannelforTopic shouldn't be public
> HedwigSubscriber#removeChannelForTopic shound't be public
>
> MessageHandler#consume should be called 'deliver'
>
> The hedwig client is under a netty package. There's nothing netty  
> specific about the api, so it should be in the  
> org.apache.hedwig.client package.
>
> In both BK and HW, context objects are passed around with callbacks.  
> I don't think this is necessary, but it doesn't bug me too much.
>
>
> -Ivan

flavio
junqueira

research scientist

fpj@yahoo-inc.com
direct +34 93-183-8828

avinguda diagonal 177, 8th floor, barcelona, 08018, es
phone (408) 349 3300    fax (408) 349 3301