[jira] [Commented] (BOOKKEEPER-422) Simplify AbstractSubscriptionManager
[ https://issues.apache.org/jira/browse/BOOKKEEPER-422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13477790#comment-13477790 ] Hudson commented on BOOKKEEPER-422: --- Integrated in bookkeeper-trunk #757 (See [https://builds.apache.org/job/bookkeeper-trunk/757/]) BOOKKEEPER-422: Simplify AbstractSubscriptionManager (stu via fpj) (Revision 1399159) Result = UNSTABLE fpj : Files : * /zookeeper/bookkeeper/trunk/CHANGES.txt * /zookeeper/bookkeeper/trunk/hedwig-server/src/main/java/org/apache/hedwig/server/subscriptions/AbstractSubscriptionManager.java Simplify AbstractSubscriptionManager Key: BOOKKEEPER-422 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-422 Project: Bookkeeper Issue Type: Improvement Components: hedwig-server Reporter: Stu Hood Assignee: Stu Hood Priority: Minor Fix For: 4.2.0 Attachments: bk-422.diff, bk-422.diff, bk-422.diff It's difficult to maintain a duplicated/cached count of local subscribers, and we've experienced a few issues due to it getting out of sync with the actual set of subscribers. Since a count of local subscribers can be calculated from the top2sub2seq map, let's do that instead. -- 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
[jira] [Commented] (BOOKKEEPER-422) Simplify AbstractSubscriptionManager
[ https://issues.apache.org/jira/browse/BOOKKEEPER-422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13476078#comment-13476078 ] Stu Hood commented on BOOKKEEPER-422: - I think that the SortedMap would be premature... as mentioned above, I think we'd be better off distinguishing between subscriber types via fields on some new Subscriber object, which could additionally allow for the namespacing needed for BOOKKEEPER-433. Simplify AbstractSubscriptionManager Key: BOOKKEEPER-422 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-422 Project: Bookkeeper Issue Type: Improvement Components: hedwig-server Reporter: Stu Hood Assignee: Stu Hood Priority: Minor Attachments: bk-422.diff, bk-422.diff, bk-422.diff It's difficult to maintain a duplicated/cached count of local subscribers, and we've experienced a few issues due to it getting out of sync with the actual set of subscribers. Since a count of local subscribers can be calculated from the top2sub2seq map, let's do that instead. -- 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
[jira] [Commented] (BOOKKEEPER-422) Simplify AbstractSubscriptionManager
[ https://issues.apache.org/jira/browse/BOOKKEEPER-422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13476082#comment-13476082 ] Flavio Junqueira commented on BOOKKEEPER-422: - bq. I think that the SortedMap would be premature How do you feel about committing this patch and changing to a SortedMap later if it becomes a bottleneck, Sijie? Is it going to affect your applications if we have this in? Simplify AbstractSubscriptionManager Key: BOOKKEEPER-422 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-422 Project: Bookkeeper Issue Type: Improvement Components: hedwig-server Reporter: Stu Hood Assignee: Stu Hood Priority: Minor Attachments: bk-422.diff, bk-422.diff, bk-422.diff It's difficult to maintain a duplicated/cached count of local subscribers, and we've experienced a few issues due to it getting out of sync with the actual set of subscribers. Since a count of local subscribers can be calculated from the top2sub2seq map, let's do that instead. -- 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
[jira] [Commented] (BOOKKEEPER-422) Simplify AbstractSubscriptionManager
[ https://issues.apache.org/jira/browse/BOOKKEEPER-422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13476097#comment-13476097 ] Sijie Guo commented on BOOKKEEPER-422: -- I am OK with this patch. +1. Simplify AbstractSubscriptionManager Key: BOOKKEEPER-422 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-422 Project: Bookkeeper Issue Type: Improvement Components: hedwig-server Reporter: Stu Hood Assignee: Stu Hood Priority: Minor Attachments: bk-422.diff, bk-422.diff, bk-422.diff It's difficult to maintain a duplicated/cached count of local subscribers, and we've experienced a few issues due to it getting out of sync with the actual set of subscribers. Since a count of local subscribers can be calculated from the top2sub2seq map, let's do that instead. -- 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
[jira] [Commented] (BOOKKEEPER-422) Simplify AbstractSubscriptionManager
[ https://issues.apache.org/jira/browse/BOOKKEEPER-422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13475068#comment-13475068 ] Flavio Junqueira commented on BOOKKEEPER-422: - It sounds like a good idea to me to use a SortedMap, Sijie. Do you see a problem with doing it, Stu? It also sounds like a good idea to validate the subscriber id as you point out, Sijie. It should be a separate jira as you suggest. Simplify AbstractSubscriptionManager Key: BOOKKEEPER-422 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-422 Project: Bookkeeper Issue Type: Improvement Components: hedwig-server Reporter: Stu Hood Assignee: Stu Hood Priority: Minor Attachments: bk-422.diff, bk-422.diff, bk-422.diff It's difficult to maintain a duplicated/cached count of local subscribers, and we've experienced a few issues due to it getting out of sync with the actual set of subscribers. Since a count of local subscribers can be calculated from the top2sub2seq map, let's do that instead. -- 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
[jira] [Commented] (BOOKKEEPER-422) Simplify AbstractSubscriptionManager
[ https://issues.apache.org/jira/browse/BOOKKEEPER-422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13474008#comment-13474008 ] Sijie Guo commented on BOOKKEEPER-422: -- if a hub server owns millions of topics, each topic has one local subscribers and 3 non-local subscribers, each time #hasLocalSubscriptions needs go thru 4 subscribers to lookup one. but it might be a problem although. I was thinking if it is possibile to change HashMap (used to store subscribers) to SortedMap, so we could cluster non-local subscribers together since they are started with underscore. it might help improving at some case. Besides this jira, I just found that it seems we don't validate the subscriber id. If a local subscriber provides a subscriber id prefixed with underscore, it might be treated as a non-local subscriber. it might be a problem. If it is a problem, we need a separated jira for it. Simplify AbstractSubscriptionManager Key: BOOKKEEPER-422 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-422 Project: Bookkeeper Issue Type: Improvement Components: hedwig-server Reporter: Stu Hood Assignee: Stu Hood Priority: Minor Attachments: bk-422.diff, bk-422.diff, bk-422.diff It's difficult to maintain a duplicated/cached count of local subscribers, and we've experienced a few issues due to it getting out of sync with the actual set of subscribers. Since a count of local subscribers can be calculated from the top2sub2seq map, let's do that instead. -- 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
[jira] [Commented] (BOOKKEEPER-422) Simplify AbstractSubscriptionManager
[ https://issues.apache.org/jira/browse/BOOKKEEPER-422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13473063#comment-13473063 ] Stu Hood commented on BOOKKEEPER-422: - bq. I am not sure this change does better, since each time #hasLocalSubscriptions needs to go thru all the subscriptions. The javadoc on that method explains that it exits as soon as it finds a non-local subscription, which should be very, very quickly (unless cross-region subscribers vastly outnumber local subscribers.) Simplify AbstractSubscriptionManager Key: BOOKKEEPER-422 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-422 Project: Bookkeeper Issue Type: Improvement Components: hedwig-server Reporter: Stu Hood Assignee: Stu Hood Priority: Minor Attachments: bk-422.diff, bk-422.diff It's difficult to maintain a duplicated/cached count of local subscribers, and we've experienced a few issues due to it getting out of sync with the actual set of subscribers. Since a count of local subscribers can be calculated from the top2sub2seq map, let's do that instead. -- 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
[jira] [Commented] (BOOKKEEPER-422) Simplify AbstractSubscriptionManager
[ https://issues.apache.org/jira/browse/BOOKKEEPER-422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13473122#comment-13473122 ] Sijie Guo commented on BOOKKEEPER-422: -- thanks Stu for clarify. lgtm. could you generate a new patch with 4-spaces indent? I saw some places are two spaces indent. Simplify AbstractSubscriptionManager Key: BOOKKEEPER-422 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-422 Project: Bookkeeper Issue Type: Improvement Components: hedwig-server Reporter: Stu Hood Assignee: Stu Hood Priority: Minor Attachments: bk-422.diff, bk-422.diff It's difficult to maintain a duplicated/cached count of local subscribers, and we've experienced a few issues due to it getting out of sync with the actual set of subscribers. Since a count of local subscribers can be calculated from the top2sub2seq map, let's do that instead. -- 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
[jira] [Commented] (BOOKKEEPER-422) Simplify AbstractSubscriptionManager
[ https://issues.apache.org/jira/browse/BOOKKEEPER-422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13473232#comment-13473232 ] Flavio Junqueira commented on BOOKKEEPER-422: - I don't mean to complicate this patch, but I was wondering if it makes sense to split the subscription state into local and non-local. This way #hasLocalSubscriptions can be implemented efficiently and it would be independent of assumptions about the distribution of subscriptions. One way would be to split top2sub2seq into two data structures, one for local and one for non-local. Even though Stu's argument makes sense, we may end up hitting corner cases and if we can avoid them altogether, better for us. Let me know if you guys think it makes sense. Simplify AbstractSubscriptionManager Key: BOOKKEEPER-422 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-422 Project: Bookkeeper Issue Type: Improvement Components: hedwig-server Reporter: Stu Hood Assignee: Stu Hood Priority: Minor Attachments: bk-422.diff, bk-422.diff It's difficult to maintain a duplicated/cached count of local subscribers, and we've experienced a few issues due to it getting out of sync with the actual set of subscribers. Since a count of local subscribers can be calculated from the top2sub2seq map, let's do that instead. -- 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
[jira] [Commented] (BOOKKEEPER-422) Simplify AbstractSubscriptionManager
[ https://issues.apache.org/jira/browse/BOOKKEEPER-422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13473538#comment-13473538 ] Stu Hood commented on BOOKKEEPER-422: - bq. One way would be to split top2sub2seq into two data structures, one for local and one for non-local. Unless the number of regions begins to get into the thousands this will not be necessary for performance reasons. And in terms of clarity, I'm thinking that the long term direction will be that we have a subscriber-specific data structure of some kind which would make the string parsing unnecessary, and further clarify the 'hasLocalSubscriptions' call. So, IMO: splitting the map before we really need to would be premature. Simplify AbstractSubscriptionManager Key: BOOKKEEPER-422 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-422 Project: Bookkeeper Issue Type: Improvement Components: hedwig-server Reporter: Stu Hood Assignee: Stu Hood Priority: Minor Attachments: bk-422.diff, bk-422.diff, bk-422.diff It's difficult to maintain a duplicated/cached count of local subscribers, and we've experienced a few issues due to it getting out of sync with the actual set of subscribers. Since a count of local subscribers can be calculated from the top2sub2seq map, let's do that instead. -- 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
[jira] [Commented] (BOOKKEEPER-422) Simplify AbstractSubscriptionManager
[ https://issues.apache.org/jira/browse/BOOKKEEPER-422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13473618#comment-13473618 ] Flavio Junqueira commented on BOOKKEEPER-422: - sounds good, +1. Simplify AbstractSubscriptionManager Key: BOOKKEEPER-422 URL: https://issues.apache.org/jira/browse/BOOKKEEPER-422 Project: Bookkeeper Issue Type: Improvement Components: hedwig-server Reporter: Stu Hood Assignee: Stu Hood Priority: Minor Attachments: bk-422.diff, bk-422.diff, bk-422.diff It's difficult to maintain a duplicated/cached count of local subscribers, and we've experienced a few issues due to it getting out of sync with the actual set of subscribers. Since a count of local subscribers can be calculated from the top2sub2seq map, let's do that instead. -- 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
[jira] [Commented] (BOOKKEEPER-422) Simplify AbstractSubscriptionManager
[
https://issues.apache.org/jira/browse/BOOKKEEPER-422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13472537#comment-13472537
]
Flavio Junqueira commented on BOOKKEEPER-422:
-
It mostly looks good to me. The only comment I have is that this patch removes
this:
{code}
-CallbackVoid noopCallback = new NoopCallbackVoid();
-
-static class NoopCallbackT implements CallbackT {
{code}
and similar patterns are used in ConsumerHandler and ChannelTracker. Should we
consider making similar changes there too and perhaps in a separate jira just
so that we don't mix the issues?
Simplify AbstractSubscriptionManager
Key: BOOKKEEPER-422
URL: https://issues.apache.org/jira/browse/BOOKKEEPER-422
Project: Bookkeeper
Issue Type: Improvement
Components: hedwig-server
Reporter: Stu Hood
Assignee: Stu Hood
Priority: Minor
Attachments: bk-422.diff
It's difficult to maintain a duplicated/cached count of local subscribers,
and we've experienced a few issues due to it getting out of sync with the
actual set of subscribers. Since a count of local subscribers can be
calculated from the top2sub2seq map, let's do that instead.
--
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
