Conversation
|
@lizhanhui, Thanks for the contribution, just noticed you closed old pr and created a new one, when you make changes to your branch, github automatically adds them to existing PR, no need to create a new PR. |
There was a problem hiding this comment.
I dont think you need the RocketMQ version number in the name here.
There was a problem hiding this comment.
OK, Will drop the version.
|
At a high level I think this PR is pretty good. A few comments overall. |
There was a problem hiding this comment.
let's use camel casing
"rocketMQ"
|
👍. I think this module will not impact much in Druid and is solving the use cases of an organization |
|
PR is void of unit tests |
2. Change RocketMQ to rocketMQ 3. Make swapRequests methods synchronized in all places. 4. Make comparator static and final and use Long.compare.
|
@drcrallen "PR is void of unit tests" Considering this module is sort of connector, it's unlikely to provide really useful unit tests without setting up messaging system and druid. Also, I just checked kafka-eight module, it lacks unit test too. |
… onWaitEnd(), is only invoked in synchronized code blocks. There is no need to repeat sync it even if java's synchronized keyword holds a reentrant mutex semantic ^_^.
|
@drcrallen as per our discussion in the dev sync, I think we can merge these extensions and look into breaking them into a separate repo |
There was a problem hiding this comment.
Should be able to just log error
There was a problem hiding this comment.
Also same comment about Thread.currentThread.interrupt() and bubbling up the interruption in a runtime exception. (throw Throwables.propogate(e))
There was a problem hiding this comment.
"Should be able to just log error" I originally directly use sl4j APIs which does not have such interface. Will update in case your logging wrapper provides.
"Also same comment about Thread.currentThread.interrupt() and bubbling up the interruption in a runtime exception. (throw Throwables.propogate(e))" you mean throw the interrupt exception up?
There was a problem hiding this comment.
Call Thread.currentThread.interrupt() and throw the exception.
Nowhere in Druid is an Interrupted thread a good thing.
|
I have a lot of questions about synchronization and lack of timeouts on polling and locking, but given that this is an isolated extension, I'm good with it being an experimental feature until either more unit tests are added or extensions are moved to their own repository. Logging comments need addressed though or else the exceptions will get eaten up by the logging framework. |
|
@drcrallen is that a +1 ? |
|
@fjy Logging comments need addressed first. |
|
@lizhanhui If you get the logging stuff flipped around I think we're good here |
|
"I have a lot of questions about synchronization and lack of timeouts on polling and locking, but given that this is an isolated extension, I'm good with it being an experimental feature until either more unit tests are added or extensions are moved to their own repository. Logging comments need addressed though or else the exceptions will get eaten up by the logging framework." synchronization issues? Please raise them up. I do not see any. " but given that this is an isolated extension, I'm good with it being an experimental feature until either more unit tests are added or extensions are moved to their own repository. Logging comments need addressed though or else the exceptions will get eaten up by the logging framework." As I previously replied, I wish I could add some unit tests that make sense. Would you recommend a way to do this without setting up MQ and Druid system? |
There was a problem hiding this comment.
Does MQClientException have any stuff that's helpful that could be logged here?
There was a problem hiding this comment.
Yes, in case something is wrong. These info are pretty helpful to figure out what's going on, where the message consuming moves to. Anyway, if the caller of this connector has no chance logging these info down, we'd better do this.
There was a problem hiding this comment.
I mean, should e be used in the error message to enhance the logging? Either as LOGGER.error(e or using e's getMessage()?
|
Actually , looking at com.alibaba.rocketmq.common.ServiceThread from http://grepcode.com/file/repo1.maven.org/maven2/com.alibaba.rocketmq/rocketmq-common/3.2.6/com/alibaba/rocketmq/common/ServiceThread.java/ it looks like ServiceThread does a lot of synchronization and notifying on itself. That makes this impl much harder to review without an intimate understanding of ServiceThread. Regarding testing, The general recommendation if the framework does not have a unit-test friendly tool, is to use EasyMock to force the behaviors you are testing for. |
|
Yes, I admit understanding ServiceThread is required. Fortunately, it's not that complex...5 min should suffice. Sure, we may mock a few cases. But really limited few. How much can we get? maybe much fewer than a real test deployment. |
There was a problem hiding this comment.
Is this just synchronizing and calling
this.hasNotified = false;
this.onWaitEnd();
?
|
@lizhanhui Thanks for the PR. Since this is intended to be used by public, can we have some documentation that describes how to use this extension? |
|
@guobingkun @drcrallen going to merge this in 24 hours and then refactor extensions |
|
This is probably served better in extensions.contrib than in a PR, so 👍 to go into extensions.contrib |
6 participants
Re-base on top of lastest master branch of druid-io repo, aka, d0b10c2