sergey-chugunov-1985 commented on code in PR #13148: URL: https://github.com/apache/ignite/pull/13148#discussion_r3302950279
########## modules/core/src/main/java/org/apache/ignite/internal/util/worker/queue/AsyncQueueHandler.java: ########## @@ -0,0 +1,179 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.util.worker.queue; + +import java.util.Iterator; +import java.util.concurrent.BlockingQueue; +import java.util.concurrent.TimeUnit; +import java.util.function.Consumer; +import org.apache.ignite.IgniteLogger; +import org.apache.ignite.internal.managers.communication.GridIoPolicy; +import org.apache.ignite.internal.thread.context.OperationContext; +import org.apache.ignite.internal.thread.context.OperationContextSnapshot; +import org.apache.ignite.internal.thread.context.function.OperationContextAwareWrapper; +import org.apache.ignite.internal.util.typedef.F; +import org.apache.ignite.internal.util.worker.GridWorker; +import org.apache.ignite.internal.worker.WorkersRegistry; +import org.apache.ignite.thread.IgniteThread; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +import static org.apache.ignite.thread.IgniteThread.GRP_IDX_UNASSIGNED; + +/** + * Represents a single-threaded, asynchronous queue elements handler. It automatically captures the {@link OperationContext} + * attached to the thread that submitted the item for handling and restores it before handling actually begins in the + * worker thread. + * + * @param <T> Type of items to be processed. + * @param <W> Type of wrapper over processing item that are stored in the underlying queue. + */ +abstract class AsyncQueueHandler<T, W extends OperationContextAwareWrapper<T>> extends GridWorker { + /** */ + private final BlockingQueue<W> workerQueue; + + /** */ + private Thread workerThread; + + /** */ + protected AsyncQueueHandler( + @Nullable String igniteInstanceName, + String workerThreadName, + IgniteLogger log, + @Nullable WorkersRegistry workerReg, + BlockingQueue<W> workerQueue + ) { + super(igniteInstanceName, workerThreadName, log, workerReg); + + this.workerQueue = workerQueue; + } + + /** */ + protected abstract W wrapQueueElement(T delegate, OperationContextSnapshot snapshot); + + /** */ + protected Thread.UncaughtExceptionHandler uncaughtExceptionHandler() { + return null; + } + + /** */ + protected IgniteThread createWorkerThread(GridWorker worker) { + return new IgniteThread(igniteInstanceName(), name(), worker, GRP_IDX_UNASSIGNED, -1, GridIoPolicy.UNDEFINED); Review Comment: We could use another IgniteThread constructor with only three arguments. grpIdx, stripe and plc parameters are always the same. ########## modules/core/src/main/java/org/apache/ignite/internal/managers/discovery/GridDiscoveryManager.java: ########## @@ -2837,68 +2818,50 @@ public DiscoveryMessageNotifierThread(GridWorker worker) { } } - /** - * - */ - private class DiscoveryMessageNotifierWorker extends GridWorker { - /** Queue. */ - private final BlockingQueue<T2<GridFutureAdapter, Runnable>> queue = new LinkedBlockingQueue<>(); - - /** - * Default constructor. - */ - protected DiscoveryMessageNotifierWorker() { + /** */ + private class DiscoveryMessageNotifier extends IgniteAsyncObjectHandler<T2<GridFutureAdapter<?>, Runnable>> { + /** Default constructor. */ + protected DiscoveryMessageNotifier() { super(ctx.igniteInstanceName(), "disco-notifier-worker", GridDiscoveryManager.this.log, ctx.workersRegistry()); } - /** - * - */ + /** {@inheritDoc} */ + @Override public IgniteThread createWorkerThread(GridWorker worker) { + return new DiscoveryMessageNotifierThread(worker); + } + + /** */ private void body0() throws InterruptedException { - T2<GridFutureAdapter, Runnable> notification; + OperationContextAwareWrapper<T2<GridFutureAdapter<?>, Runnable>> contextualNotification = takeQueuedElement(); - blockingSectionBegin(); + try (Scope ignored = OperationContext.restoreSnapshot(contextualNotification.contextSnapshot())) { + T2<GridFutureAdapter<?>, Runnable> notification = contextualNotification.delegate(); - try { - notification = queue.take(); - } - finally { - blockingSectionEnd(); - } - - try { - notification.get2().run(); - } - finally { - notification.get1().onDone(); + try { + notification.get2().run(); + } + finally { + notification.get1().onDone(); + } } } - /** - * @param cmd Command. - */ - public synchronized void submit(GridFutureAdapter notificationFut, Runnable cmd) { + /** @param cmd Command. */ + public synchronized void submit(GridFutureAdapter<?> notificationFut, Runnable cmd) { if (isCancelled()) { notificationFut.onDone(); return; } - queue.add(new T2<>(notificationFut, cmd)); + addToQueue(new T2<>(notificationFut, cmd)); } - /** - * Cancel thread execution and completes all notification futures. - */ + /** Cancel thread execution and completes all notification futures. */ Review Comment: ```suggestion /** Cancels thread execution and completes all notification futures. */ ``` ########## modules/core/src/main/java/org/apache/ignite/internal/processors/datastreamer/DataStreamProcessor.java: ########## @@ -190,6 +164,16 @@ public DataStreamerImpl<K, V> dataStreamer(@Nullable String cacheName) { } } + /** */ + public void scheduleAutoFlush(DataStreamerImpl<?, ?> dataStreamer) { Review Comment: Why two methods exposed through GridKernalContext instead of passing flusher to DataStreamerImpl? Both approaches are not ideal but the last one is less invasive from my point of view. ########## modules/core/src/main/java/org/apache/ignite/internal/util/worker/queue/AsyncQueueHandler.java: ########## @@ -0,0 +1,179 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.util.worker.queue; + +import java.util.Iterator; +import java.util.concurrent.BlockingQueue; +import java.util.concurrent.TimeUnit; +import java.util.function.Consumer; +import org.apache.ignite.IgniteLogger; +import org.apache.ignite.internal.managers.communication.GridIoPolicy; +import org.apache.ignite.internal.thread.context.OperationContext; +import org.apache.ignite.internal.thread.context.OperationContextSnapshot; +import org.apache.ignite.internal.thread.context.function.OperationContextAwareWrapper; +import org.apache.ignite.internal.util.typedef.F; +import org.apache.ignite.internal.util.worker.GridWorker; +import org.apache.ignite.internal.worker.WorkersRegistry; +import org.apache.ignite.thread.IgniteThread; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +import static org.apache.ignite.thread.IgniteThread.GRP_IDX_UNASSIGNED; + +/** + * Represents a single-threaded, asynchronous queue elements handler. It automatically captures the {@link OperationContext} + * attached to the thread that submitted the item for handling and restores it before handling actually begins in the + * worker thread. + * + * @param <T> Type of items to be processed. + * @param <W> Type of wrapper over processing item that are stored in the underlying queue. + */ +abstract class AsyncQueueHandler<T, W extends OperationContextAwareWrapper<T>> extends GridWorker { + /** */ + private final BlockingQueue<W> workerQueue; + + /** */ + private Thread workerThread; + + /** */ + protected AsyncQueueHandler( + @Nullable String igniteInstanceName, + String workerThreadName, + IgniteLogger log, + @Nullable WorkersRegistry workerReg, + BlockingQueue<W> workerQueue + ) { + super(igniteInstanceName, workerThreadName, log, workerReg); + + this.workerQueue = workerQueue; + } + + /** */ + protected abstract W wrapQueueElement(T delegate, OperationContextSnapshot snapshot); + + /** */ + protected Thread.UncaughtExceptionHandler uncaughtExceptionHandler() { + return null; + } + + /** */ + protected IgniteThread createWorkerThread(GridWorker worker) { + return new IgniteThread(igniteInstanceName(), name(), worker, GRP_IDX_UNASSIGNED, -1, GridIoPolicy.UNDEFINED); + } + + /** */ + @Nullable protected OperationContextAwareWrapper<T> takeQueuedElement() throws InterruptedException { + blockingSectionBegin(); + + try { + return workerQueue.take(); + } + finally { + blockingSectionEnd(); + } + } + + /** */ + @Nullable protected OperationContextAwareWrapper<T> pollQueuedElement( + long timeout, + @NotNull TimeUnit unit + ) throws InterruptedException { + blockingSectionBegin(); + + try { + return workerQueue.poll(timeout, unit); + } + finally { + blockingSectionEnd(); + } + } + + /** */ + public void start() { + synchronized (this) { + if (workerThread != null) + return; + + workerThread = createWorkerThread(this); + + Thread.UncaughtExceptionHandler errHnd = uncaughtExceptionHandler(); + + if (errHnd != null) + workerThread.setUncaughtExceptionHandler(errHnd); + + workerThread.start(); + } + } + + /** {@inheritDoc} */ + @Override protected void cleanup() { Review Comment: I guess cleanup method is called as part of node's stop procedure and race with start method is not possible? By race I mean a situation when the first thread calls start method, the second on calls cleanup and then the third calls start again and reverts the work of the second thread. If this situation is not possible it is fine to ignore this comment. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
