----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6277/#review12223 -----------------------------------------------------------
Overall the patch looks good and clearly outlines the current limitations. Here are some general comments (more comments are available as annotations on the code): - It looks like the tests expect some instance of hedwig to be running somewhere. Is this the case? They should rather run out of the box, mocking hedwig server if necessary or possible. hedwig-client-jms/src/main/java/org/apache/hedwig/jms/SessionImpl.java <https://reviews.apache.org/r/6277/#comment25941> Parameters in Hedwig/Bookkeeper are usually read from ClientConfiguration, it might be worth reusing it. As mentioned by Sije, a composite configuration could actually include parameters fetched from JNDI. hedwig-client-jms/src/main/java/org/apache/hedwig/jms/SessionImpl.java <https://reviews.apache.org/r/6277/#comment25942> Better to name the thread hedwig-client-jms/src/main/java/org/apache/hedwig/jms/spi/HedwigMessagingSessionFacade.java <https://reviews.apache.org/r/6277/#comment25939> "queues" instead of "queue's" hedwig-client-jms/src/main/java/org/apache/hedwig/jms/spi/MessageConsumerImpl.java <https://reviews.apache.org/r/6277/#comment25940> underscore-prefixing the parameter name is not a convention used in bookkeeper/hedwig "this." already identifies the homonymic field hedwig-client-jms/src/test/java/org/apache/hedwig/jms/BasicJMSTest.java <https://reviews.apache.org/r/6277/#comment25938> do not use assert java keyword for evaluating test results. Instead, use JUnit's Assert functions (also occurs in other places in this file) - mmorel On Aug. 10, 2012, 5:53 a.m., Mridul wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6277/ > ----------------------------------------------------------- > > (Updated Aug. 10, 2012, 5:53 a.m.) > > > Review request for bookkeeper. > > > Description > ------- > > > Add support for JMS provider conforming to 1.1 as detailed in BOOKKEEPER-312. > > The limitations are (as detailed in the bug) : > > 0) Due to lack of proper support for consume in face of disconnect, some > tests fail - this has to be addressed in hedwig independent of this bug. > > 1) Need better ways to connect to server - currently it is (sort of) > hardcoded via property file(s). > Also, need better ways to authorize to server (assuming hedwig supports this). > A subsequent bug can extend/enhance the provider to add support for this > (JAAS or whatever is expected to be supported). > > 2) No support for Queue's. > > 3) No support for noLocal : simulating it in provider. > > 4) We do not support NON_PERSISTENT delivery mode. > > 5) Calling unsubscribe on a durable subscription will fail if it was NOT > created in the current session. > > 6) Explicit session recovery is not supported. > and so setting the JMSRedelivered flag is simulated (best case effort). > > 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 addresses bug BOOKKEEPER-312. > https://issues.apache.org/jira/browse/BOOKKEEPER-312 > > > Diffs > ----- > > hedwig-client-jms/pom.xml PRE-CREATION > hedwig-client-jms/src/main/grammar/javacc/readme.html PRE-CREATION > hedwig-client-jms/src/main/grammar/javacc/selector_grammar.jjt PRE-CREATION > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/ConnectionImpl.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/ConnectionMetaDataImpl.java > PRE-CREATION > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/DebugUtil.java > PRE-CREATION > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/LRUCacheMap.java > PRE-CREATION > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/LRUCacheSet.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/MessagingSessionFacade.java > PRE-CREATION > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/Mutable.java > PRE-CREATION > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/SessionImpl.java > PRE-CREATION > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/StateManager.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/jndi/HedwigInitialContext.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/jndi/HedwigInitialContextFactory.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/jndi/package-info.html > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/message/BytesMessageImpl.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/message/MapMessageImpl.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/message/MessageImpl.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/message/MessageUtil.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/message/ObjectMessageImpl.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/message/StreamMessageImpl.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/message/TextMessageImpl.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/message/header/JmsHeader.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/message/header/MetadataProcessor.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/message/package-info.html > PRE-CREATION > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/package-info.html > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/selector/BinaryArithmeticFunction.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/selector/BinaryExprFunction.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/selector/ExprFunction.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/selector/InterpretSelectorParserVisitor.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/selector/LogicalComparisonFunction.java > PRE-CREATION > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/selector/MyNode.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/selector/PropertyExprFunction.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/selector/SelectorConstant.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/selector/SelectorEvalState.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/selector/SelectorEvaluationException.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/selector/TreeDumperSelectorParserVisitor.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/selector/UnaryArithmeticFunction.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/selector/UnaryExprFunction.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/selector/ValueComparisonFunction.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/selector/package-info.html > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/spi/HedwigConnectionFactoryImpl.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/spi/HedwigConnectionImpl.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/spi/HedwigMessagingSessionFacade.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/spi/MessageConsumerImpl.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/spi/MessageProducerImpl.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/spi/QueueSessionImpl.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/spi/TopicPublisherImpl.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/spi/TopicSessionImpl.java > PRE-CREATION > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/spi/TopicSubscriberImpl.java > PRE-CREATION > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/spi/package-info.html > PRE-CREATION > hedwig-client-jms/src/main/protobuf/JmsHeader.proto PRE-CREATION > hedwig-client-jms/src/main/resources/log4j.properties PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/AutoFailTestSupport.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/CombinationTestSupport.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/ConnectionCleanupTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/ConsumerReceiveWithTimeoutTest.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/ExpiryHogTest.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/JMSConsumerTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JMSDurableTopicRedeliverTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JMSIndividualAckTest.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/JMSMessageTest.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/JMSUsecaseTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsAutoAckListenerTest.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/JmsAutoAckTest.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/JmsBenchmark.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsClientAckListenerTest.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/JmsClientAckTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsConnectionStartStopTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsConsumerResetActiveListenerTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsCreateConsumerInOnMessageTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsDurableTopicSelectorTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsDurableTopicSendReceiveTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsDurableTopicTransactionTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsMultipleClientsTestSupport.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/JmsRedeliveredTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsRollbackRedeliveryTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsSendReceiveTestSupport.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsSendReceiveWithMessageExpirationTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsSendWithAsyncCallbackTest.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/JmsTestSupport.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsTopicCompositeSendReceiveTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsTopicRedeliverTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsTopicSelectorTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsTopicSendReceiveSubscriberTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsTopicSendReceiveTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsTopicSendReceiveWithTwoConnectionsTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsTopicSendReceiveWithTwoConnectionsWithJMXTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsTopicSendSameMessageTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsTopicTransactionTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/JmsTransactionTestSupport.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/LoadTestBurnIn.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/MessageListenerRedeliveryTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/RedeliveryPolicyTest.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/TestSupport.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/TimeStampTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/demo/SimpleConsumer.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/demo/SimpleProducer.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/load/LoadClient.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/load/LoadController.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/load/LoadTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/perf/ConnectionChurnTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/perf/InactiveDurableTopicTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/perf/NetworkedSyncTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/perf/NumberOfDestinationsTest.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/perf/PerfConsumer.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/perf/PerfProducer.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/perf/PerfRate.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/perf/SimpleDurableTopicNetworkTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/perf/SimpleDurableTopicTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/perf/SimpleNetworkTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/perf/SimpleNonPersistentTopicTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/perf/SimpleTopicTest.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/perf/SlowConsumer.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/perf/SlowConsumerTopicTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/perf/SlowDurableConsumerTopicTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/test/JmsResourceProvider.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/test/JmsSendReceiveTestSupport.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/test/JmsTopicSendReceiveTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/test/JmsTopicSendReceiveWithTwoConnectionsAndByteSelectorTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/test/JmsTopicSendReceiveWithTwoConnectionsTest.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/test/TestSupport.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/test/rollback/DelegatingTransactionalMessageListener.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/ChangeSentMessageTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/ChangeSessionDeliveryModeTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/CompositeConsumeTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/CompositePublishTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/ConcurrentProducerDurableConsumerTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/DiscriminatingConsumerLoadTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/DispatchMultipleConsumersTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/DurableConsumerCloseAndReconnectTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/DurableSubProcessTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/DurableSubSelectorDelayTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/DurableSubscriptionHangTestCase.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/MyObject.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/NonBlockingConsumerRedeliveryTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/ObjectMessageNotSerializableTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/ProducerConsumerTestSupport.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/PublishOnDurableTopicConsumedMessageTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/PublishOnTopicConsumedMessageTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/SubscribeClosePublishThenConsumeTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/TestSupport.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/TopicRedeliverTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/TransactionRollbackOrderTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/usecases/TransactionTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/util/ConsumerThread.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/util/DefaultTestAppender.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/util/IdGenerator.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/util/MessageIdList.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/activemq/util/ProducerThread.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/util/SimplePojo.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/activemq/util/Wait.java > PRE-CREATION > hedwig-client-jms/src/test/java/org/apache/hedwig/jms/BasicJMSTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/hedwig/jms/selector/BasicSelectorGrammarTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/hedwig/jms/selector/activemq/SelectorParserTest.java > PRE-CREATION > > hedwig-client-jms/src/test/java/org/apache/hedwig/jms/selector/activemq/SelectorTest.java > PRE-CREATION > pom.xml 9cea6ae > > Diff: https://reviews.apache.org/r/6277/diff/ > > > Testing > ------- > > > Thanks, > > Mridul > >
