You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/12/14 21:51:41 UTC
[GitHub] [helix] xyuanlu commented on a change in pull request #1589: Reduce lock contention in CallbackHandler.enqueueTask
xyuanlu commented on a change in pull request #1589:
URL: https://github.com/apache/helix/pull/1589#discussion_r542834510
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -766,16 +769,16 @@ void reset(boolean isShutdown) {
isShutdown);
try {
_ready = false;
- synchronized (this) {
- if (_batchCallbackProcessor != null) {
+ CallbackProcessor callbackProcessorRef = _batchCallbackProcessor.get();
Review comment:
Sorry I did not quite follow here. We still need to do nullptr check of the return value from getAndSet and also need to check isShutdown in the outer if. It does not seems to make this embedded if simpler.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -643,14 +643,17 @@ public void init() {
logger.info("initializing CallbackHandler: {}, content: {} ", _uid, getContent());
if (_batchModeEnabled) {
- synchronized (this) {
- if (_batchCallbackProcessor != null) {
- _batchCallbackProcessor.resetEventQueue();
+ CallbackProcessor callbackProcessorRef = _batchCallbackProcessor.get();
+ if (callbackProcessorRef != null) {
+ callbackProcessorRef.resetEventQueue();
} else {
- _batchCallbackProcessor = new CallbackProcessor(this);
- _batchCallbackProcessor.start();
+ callbackProcessorRef = new CallbackProcessor(this);
+ if (_batchCallbackProcessor.compareAndSet(null, callbackProcessorRef)) {
+ callbackProcessorRef.start();
+ } else {
+ callbackProcessorRef.shutdown();
Review comment:
I think `new CallbackProcessor(this)` creates a new thread and we need to close it if the new thread is not updated to `_batchCallbackProcessor`. However, since are doing start and then insert now, we need this `shutdown` anyway.
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java
##########
@@ -643,14 +643,17 @@ public void init() {
logger.info("initializing CallbackHandler: {}, content: {} ", _uid, getContent());
if (_batchModeEnabled) {
- synchronized (this) {
- if (_batchCallbackProcessor != null) {
- _batchCallbackProcessor.resetEventQueue();
+ CallbackProcessor callbackProcessorRef = _batchCallbackProcessor.get();
+ if (callbackProcessorRef != null) {
+ callbackProcessorRef.resetEventQueue();
} else {
- _batchCallbackProcessor = new CallbackProcessor(this);
- _batchCallbackProcessor.start();
+ callbackProcessorRef = new CallbackProcessor(this);
+ if (_batchCallbackProcessor.compareAndSet(null, callbackProcessorRef)) {
+ callbackProcessorRef.start();
Review comment:
TFTR. I think you are correct. We should't touch the `callbackProcessorRef ` object after `compareAndSet` returns true.
Need to start and then insert
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org