> On Oct. 8, 2012, 4:06 p.m., mmorel wrote: > > 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. > > > > > > > > Mridul wrote: > True, (most of) the tests expect the hedwig server to be running - and > the configuration to point to this. > Note that the activemq testcases assume the same (actually, in activemq > codebase, the jndi params can bootstrap a broker - which we cant support). > > Given this, we could add that as a pre test hook (not sure which in > maven) to start the server and post test hook to shut it down ? > Since I am not too well versed in maven or hedwig/bookkeeper, I did not > pursue it too much : currently, I am manually ensuring the same ... > > A better way would greatly improve the quality of experience !
In my opinion, it is better to verify the tests by using a new Hedwig instance for each test, so that each test is self-contained, and in order to avoid side effects from previous tests. HedwigHubTestBase could be reused for that purpose, as in hedwig-server tests. (You could add a dependency to hedwig-server-tests). You might be able to mock hedwig in some cases, which would be faster, but I'm not sure how it can be done. > On Oct. 8, 2012, 4:06 p.m., mmorel wrote: > > hedwig-client-jms/src/main/java/org/apache/hedwig/jms/SessionImpl.java, > > line 83 > > <https://reviews.apache.org/r/6277/diff/2/?file=137124#file137124line83> > > > > 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. > > Mridul wrote: > Agree. > > There are a bunch of places where configuration is picked up from env > variables. > > Integer.getInteger, System.getProperty, etc. > > Once we have a better configuration story, we can handle all of these > cases as part of that change-set (which would, hopefully, be soon) : > including better authorization, multiple server (read from one or more hedwig > and write to one or more), etc. > Does that sound fine ? I think it's ok to leave that for later, but in that case you should create a new ticket for making sure the issue is tracked > On Oct. 8, 2012, 4:06 p.m., mmorel wrote: > > hedwig-client-jms/src/test/java/org/apache/hedwig/jms/BasicJMSTest.java, > > line 112 > > <https://reviews.apache.org/r/6277/diff/2/?file=137266#file137266line112> > > > > do not use assert java keyword for evaluating test results. Instead, > > use JUnit's Assert functions (also occurs in other places in this file) > > Mridul wrote: > If that is the convention followed, I will change it appropriately. > > > Thanks for the review Matthieu ! Yes, all of hedwig and bookkeeper's tests use JUnit's API. - mmorel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6277/#review12223 ----------------------------------------------------------- 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 > >
