[
https://issues.apache.org/jira/browse/BOOKKEEPER-312?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13427965#comment-13427965
]
Sijie Guo commented on BOOKKEEPER-312:
--------------------------------------
the overall patch looks great although I just taked a look at the skeleton of
the patch w/o test cases and selector parts.
I had some general comments:
1) code style. it would be better to follow Java code convention for if-else,
while, for blocks.
http://www.oracle.com/technetwork/java/codeconventions-142311.html#449
2) In general, it would be better to provide detail meaningful messages for
exception and log, not just short statement.
3) configurations. it would be better to have a configuration object for it,
even for now we passing settings thru system properties. you could reference
bookkeeper-server/AbstractConfiguration, we used a CompositeConfiguration to
add a SystemConfiguration. so it could read settings from system properties
4) log4j provide mechanism to log the stack like #logger.debug("error message",
exception). you don't need to do it separately using Util.dumpStacks you
provided.
>> comments about Selector.
I think it would be better to define a interface like filter(Message message)
to hide AST related structure under this interface. so we could decouple AST
node (like Node ast ) from main logic. And it would be easy to split the big
patch into several small parts.
>> comments about patch review:
it is a too big patch for reviewing and covering them. My suggestion is to
split it into several smaller patches.
these smaller patches could be:
1) skeleton of the code. publisher part code and test cases.
2) subscriber (consumer) part code w/o filtering and test cases. (it could be
done if we had clean interface for filtering to hide selector part as my above
comment)
3) selector part and filtering and test cases.
4) for queues, since hedwig doesn't support queue. it would be better to no
include any code about queue, just throw UnsupportOperationException for future
extension.
For this four parts, it would be better to have them in separated jiras. the
later one's patch is generated based on the previous one.
I know it introduces additional works for you, but it did help the community to
review your codes, give comments and improve code quality before any commits.
And I could help you doing the split work if the suggestion makes sense for
you.
other comments about code you could check them in review board:
https://reviews.apache.org/r/6277/
> Implementation of JMS provider
> ------------------------------
>
> Key: BOOKKEEPER-312
> URL: https://issues.apache.org/jira/browse/BOOKKEEPER-312
> Project: Bookkeeper
> Issue Type: Sub-task
> Reporter: Mridul Muralidharan
> Attachments: hedwig-client-jms.patch, hedwig-client-jms.patch.1,
> hedwig-client-jms.patch.2, hedwig-client-jms.patch.3,
> hedwig-client-jms.patch.4, hedwig-client-jms.patch.5
>
>
> The JMS provider implementation conforming to the 1.1 spec.
> The limitations as of now are :
> 1) No support for Queue's : Hedwig currently does not have a notion of JMS
> queue's for us to leverage.
> 2) No support for noLocal : Hedwig DOES NOT conform to JMS model of
> connection -(n)-> session -(n)-> publisher/subscriber. Each session has a
> hedwig connection.
> Currently I am simulating noLocal, but this IS fragile and works for the
> duration of connection - ONLY until the message id is still in a LRUCache. As
> mentioned before, this is a kludge, and not a good solution.
> 3) Note that everything is durable in hedwig - so we do not support
> NON_PERSISTENT delivery mode.
> 4) Calling unsubscribe on a durable subscription will fail if it was NOT
> created in the current session.
> In hedwig, to unsubscribe, we need the subscription id and the topic ...
> To simulate unsubscribe(), we store the subscriberId to topicName mapping
> when a create* api is invoked. Hence, if create* was NOT called, then we have
> no way to infer which topic the subscription-id refers to from hedwig, and so
> cant unsubscribe.
> The workaround is - simply create a durable subsriber just as a workaround of
> this limitation - the topicName will be known to the user/client anyway.
> 5) Explicit session recovery is not supported.
> Reconnection of hedwig session (either explicitly or implicitly by underlying
> client implementation) will automatically trigger redelivery of
> un-acknowledged messages.
> 6) Because of the above, setting the JMSRedelivered flag is almost impossible
> in a consistent way.
> Currently, we simulate it for redelivery due to provider side events :
> rollback of txn, exception in message listener (primarily).
> At best we can simulate it with a kludge - at risk of potentially running out
> of resources ... this is being investigated : but unlikely to have a clean
> fix.
> 7) Hedwig only supports marking all messages until seq-id as received : while
> JMS indicates ability to acknowledge individual messages.
> This distinction is currently unsupported.
> 8) JMS spec requires
> "A connection's delivery of incoming messages can be temporarily stopped
> using its stop() method. It can be restarted using its start() method. When
> the connection is stopped, delivery to all the connection’s MessageConsumers
> is inhibited: synchronous receives block, and messages are not delivered to
> MessageListeners."
> We honor this for undelivered messages from server - but if stop is called
> while there are pending messages yet to be delivered to a listener (or
> buffered in subscriber for receive), then they will be delivered irrespective
> of stop().
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
