[
https://issues.apache.org/jira/browse/BOOKKEEPER-362?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13483033#comment-13483033
]
Sijie Guo commented on BOOKKEEPER-362:
--------------------------------------
It seems that the new patch could not be applied to latest trunk.
I took a look at the patch and had some comments as below:
1) #checkTopicSubscribedFromRegion #setTopicUnsubscribedFromRegion
setTopicSubscribedFromRegion
1.1) where to manage these informations?
I don't think it was a good idea to TopicManager. Because TopicManager takes a
role to manage the ownership of topics, but these three method tries to
interactive with metadata related to region info, which is similar as
SubscriptionDataManager and TopicPersistenceInfoManager. So I would suggest you
to move it to a brand new metadata manager like
'TopicRemoteSubscriptionDataManager' and put this manager under
MetadataManagerFactory. And you could provide a
ZKTopicRemoteSubscriptionDataManager in ZKMetadataManagerFactory. so you don't
need to worry the TODO issue as you described in MMTopicManager. And it would
make the responsibilities more clear.
1.2) the name of these methods?
from my understanding about this issue, the data to record is what regions that
the hub has subscribed the topic remotely. so a better name might be 'To' not
'From', like '#checkTopicSubscribedToRegion' '#setTopicUnsubscribedToRegion',
'#setTopicSubscribedToRegion'. If my understanding is not right, please correct
me.
1.3) It would be better to use colo name rather than regionAddress, I think.
since regionAddress might be changed to a different address.
Besides that, ZooKeeperServiceDown is not a good name, I would suggest using
name like 'MetadataServiceDown'.
2)
{code}
- // no subscriptions now, it may be removed by other
release ops
- if (null != topicSubscriptions) {
- for (ByteString subId : topicSubscriptions.keySet()) {
- if (logger.isDebugEnabled()) {
- logger.debug("Stop serving subscriber (" +
topic.toStringUtf8() + ", "
- + subId.toStringUtf8() + ") when
losing topic");
- }
- if (null != dm) {
- dm.stopServingSubscriber(topic, subId);
- }
- }
- }
- if (logger.isDebugEnabled()) {
- logger.debug("Stop serving topic " +
topic.toStringUtf8());
- }
- // Since we decrement local count when some of remote
subscriptions failed,
- // while we don't unsubscribe those succeed subscriptions.
so we can't depends
- // on local count, just try to notify unsubscribe.
- notifyLastLocalUnsubscribe(topic);
+
cb.operationFinished(ctx, null);
{code}
seems that you removed some logic (stop serving subscriber when unsubscribe)
when you rebased the patch.
(3) It was great to clean up the boolean flag in ReleaseOp but adding backup
map. But seems that this fix is not relate to this jira. so if it was convient,
when you generate a new patch, could you split this part into a separated JIRA?
{code}
public class InMemorySubscriptionManager extends AbstractSubscriptionManager {
+ // Backup for top2sub2seq
+ final ConcurrentHashMap<ByteString, Map<ByteString,
InMemorySubscriptionState>> _top2sub2seq =
+ new ConcurrentHashMap<ByteString, Map<ByteString,
InMemorySubscriptionState>>();
{code}
(4) indent issue
{code}
if (LOGGER.isDebugEnabled())
- LOGGER.debug("[" + myRegion + "]
cross-region recv-fwd succeeded for topic "
+ LOGGER.debug("[" + myRegion + "]
cross-region recv-fwd succeeded for topic "
+ topic.toStringUtf8());
{code}
I saw some code are in wrong indent. I think it might be introduced by rebase.
> Local subscriptions fail if remote region is down
> -------------------------------------------------
>
> Key: BOOKKEEPER-362
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-362
> Project: Bookkeeper
> Issue Type: Bug
> Components: hedwig-server
> Affects Versions: 4.2.0
> Reporter: Aniruddha
> Assignee: Aniruddha
> Priority: Critical
> Labels: hedwig
> Attachments:
> 0001-Ignore-hub-client-remote-subscription-failure-if-we-.patch,
> rebase_remoteregion.patch
>
>
> Currently, local subscriptions fail if the remote region hubs are down, even
> if the local hub has subscribed to the remote topic previously. Because of
> this, one region cannot function independent of the other.
> A more detailed discussion related to this can be found here
> http://mail-archives.apache.org/mod_mbox/zookeeper-bookkeeper-dev/201208.mbox/%3cCAOLhyDQSOF+Y+pvnyrd-HJRq1YEr=c8ok_b3_mr81r1g-9m...@mail.gmail.com%3e
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
