WangzJi commented on code in PR #8023: URL: https://github.com/apache/incubator-seata/pull/8023#discussion_r3151744016
########## threadpool-loom/src/main/java/org/apache/seata/common/thread/VirtualThreadPoolExecutor.java: ########## @@ -0,0 +1,47 @@ +/* + * 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.seata.common.thread; + +import java.util.concurrent.BlockingQueue; +import java.util.concurrent.RejectedExecutionHandler; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; + +/** + * Virtual-thread-backed thread pool implementation. + */ +public class VirtualThreadPoolExecutor extends ThreadPoolExecutor { + + public VirtualThreadPoolExecutor( + String threadPrefix, + int corePoolSize, + int maximumPoolSize, + long keepAliveTime, + TimeUnit unit, + BlockingQueue<Runnable> workQueue, + boolean daemon, + RejectedExecutionHandler rejectedHandler) { + super( + corePoolSize, + maximumPoolSize, + keepAliveTime, + unit, + workQueue, + VirtualThreadFactoryHelper.newThreadFactory(threadPrefix, daemon), + rejectedHandler); Review Comment: This doesn't match the PR description, which says virtual mode ignores maximumPoolSize/keepAliveTime and uses Integer.MAX_VALUE + SynchronousQueue. Current code passes the caller's bounds straight through, and testVirtualThreadPoolExecutorKeepsConfiguredBounds locks that in. So either the impl needs to follow the description, or the description (and the test assertion) should be updated to say virtual mode still respects caller bounds. ########## threadpool-loom/src/main/java/org/apache/seata/common/thread/VirtualScheduledThreadPoolExecutor.java: ########## @@ -0,0 +1,31 @@ +/* + * 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.seata.common.thread; + +import java.util.concurrent.RejectedExecutionHandler; +import java.util.concurrent.ScheduledThreadPoolExecutor; + +/** + * Virtual-thread-backed scheduled thread pool implementation. + */ +public class VirtualScheduledThreadPoolExecutor extends ScheduledThreadPoolExecutor { + + public VirtualScheduledThreadPoolExecutor( + String threadPrefix, int corePoolSize, boolean daemon, RejectedExecutionHandler rejectedHandler) { + super(corePoolSize, VirtualThreadFactoryHelper.newThreadFactory(threadPrefix, daemon), rejectedHandler); Review Comment: ScheduledThreadPoolExecutor parks its workers in DelayedWorkQueue.take() forever, which pins the carrier when those workers are virtual threads — so we get zero loom benefit and bring back the pinning issue from #6724. ~12 migrated call sites go through this (RegistryHeartBeats, DefaultCoordinator.retryRollbacking / asyncCommitting / xaTwoPhaseTimeoutChecker, CommonFenceConfig, ...). Maybe have the loom provider delegate newScheduledThreadPoolExecutor to the platform impl? ########## threadpool/src/main/java/org/apache/seata/common/thread/ThreadPoolRuntimeEnvironment.java: ########## @@ -0,0 +1,98 @@ +/* + * 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.seata.common.thread; + +import org.apache.seata.common.ConfigurationKeys; +import org.apache.seata.common.DefaultValues; +import org.apache.seata.config.Configuration; +import org.apache.seata.config.ConfigurationFactory; + +import java.util.function.IntSupplier; +import java.util.function.Supplier; + +/** + * Runtime helper used to resolve the thread pool mode. + */ +final class ThreadPoolRuntimeEnvironment { + + private static final Supplier<String> DEFAULT_THREAD_POOL_TYPE_SUPPLIER = + ThreadPoolRuntimeEnvironment::loadConfiguredThreadPoolType; + private static final IntSupplier DEFAULT_JDK_FEATURE_SUPPLIER = ThreadPoolRuntimeEnvironment::javaFeatureVersion; + + private static volatile Supplier<String> threadPoolTypeSupplier = DEFAULT_THREAD_POOL_TYPE_SUPPLIER; + private static volatile IntSupplier jdkFeatureSupplier = DEFAULT_JDK_FEATURE_SUPPLIER; + + private ThreadPoolRuntimeEnvironment() {} + + static ThreadPoolType resolveThreadPoolType() { + ThreadPoolType configuredType = ThreadPoolType.from(threadPoolTypeSupplier.get()); + if (configuredType == ThreadPoolType.PLATFORM) { + return ThreadPoolType.PLATFORM; + } + int jdkFeature = jdkFeatureSupplier.getAsInt(); + if (configuredType == ThreadPoolType.VIRTUAL) { + return jdkFeature >= 21 ? ThreadPoolType.VIRTUAL : ThreadPoolType.PLATFORM; Review Comment: No logger anywhere in this class. If someone sets transport.threadpool=virtual on JDK <21 we silently downgrade here, and if loom isnt on the classpath ThreadPoolExecutorFactory.loadOptional (line 174) silently returns null. From the operators side theres no way to tell which mode is actually in effect. A one-shot WARN on each downgrade path would help a lot. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
