Copilot commented on code in PR #8023: URL: https://github.com/apache/incubator-seata/pull/8023#discussion_r2999173568
########## threadpool-loom/src/main/java/org/apache/seata/common/thread/VirtualThreadPoolExecutor.java: ########## @@ -0,0 +1,44 @@ +/* + * 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.SynchronousQueue; +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, boolean daemon, RejectedExecutionHandler rejectedHandler) { + super( + corePoolSize, + Integer.MAX_VALUE, + 0L, + TimeUnit.MILLISECONDS, + new SynchronousQueue<>(), + Thread.ofVirtual().name(normalizePrefix(threadPrefix), 1).factory(), + rejectedHandler); Review Comment: `VirtualThreadPoolExecutor` takes a `daemon` argument but does not (and cannot) apply it to virtual threads; this makes the constructor contract misleading for callers and for SPI implementations. Consider removing the parameter (if feasible) or at least documenting that it is ignored for virtual-thread executors. ########## threadpool-loom/src/main/java/org/apache/seata/common/thread/VirtualThreadPoolProvider.java: ########## @@ -0,0 +1,51 @@ +/* + * 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.loader.LoadLevel; + +import java.util.concurrent.BlockingQueue; +import java.util.concurrent.RejectedExecutionHandler; +import java.util.concurrent.ScheduledThreadPoolExecutor; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; + +/** + * JDK 21+ SPI implementation that creates virtual-thread-backed business pools. + */ +@LoadLevel(name = "virtual", order = Integer.MIN_VALUE) +public class VirtualThreadPoolProvider implements ThreadPoolProvider { + + @Override + public ThreadPoolExecutor newThreadPoolExecutor( + String threadPrefix, + int corePoolSize, + int maximumPoolSize, + long keepAliveTime, + TimeUnit unit, + BlockingQueue<Runnable> workQueue, + boolean daemon, + RejectedExecutionHandler rejectedHandler) { + return new VirtualThreadPoolExecutor(threadPrefix, corePoolSize, daemon, rejectedHandler); + } Review Comment: The `daemon` parameter is accepted but cannot actually be applied when using `Thread.ofVirtual()`; currently it’s silently ignored (both in the provider and executor). Please document this explicitly (e.g., Javadoc on `ThreadPoolProvider`/`VirtualThreadPoolProvider`) and/or validate/reject `daemon=false` to avoid callers assuming non-daemon behavior. ########## threadpool/src/test/java/org/apache/seata/common/thread/ThreadPoolExecutorFactoryTest.java: ########## @@ -0,0 +1,148 @@ +/* + * 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.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.ScheduledThreadPoolExecutor; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for {@link ThreadPoolExecutorFactory}. + */ +public class ThreadPoolExecutorFactoryTest { + + @AfterEach + public void tearDown() { + ThreadPoolRuntimeEnvironment.reset(); + } + + @Test + public void testNewThreadFactoryUsesDefaultProvider() { + ThreadFactory threadFactory = ThreadPoolExecutorFactory.newThreadFactory("factoryTest", 2, true); + + Thread thread = threadFactory.newThread(() -> {}); + + assertThat(thread.getName()).startsWith("factoryTest"); + assertThat(thread.isDaemon()).isTrue(); + } Review Comment: The test name `testNewThreadFactoryUsesDefaultProvider` is misleading: `ThreadPoolExecutorFactory.newThreadFactory(...)` currently bypasses `ThreadPoolProvider` selection entirely and always returns a `NamedThreadFactory`. Consider renaming the test (or updating implementation if thread-factory creation is intended to be provider-backed). ########## threadpool/src/main/java/org/apache/seata/common/thread/ThreadPoolType.java: ########## @@ -0,0 +1,62 @@ +/* + * 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.DefaultValues; +import org.apache.seata.common.util.StringUtils; + +/** + * Supported Seata thread pool modes. + */ +public enum ThreadPoolType { + /** + * Automatic selection: uses virtual threads when running on JDK 25 or later (with the loom extension + * present), otherwise falls back to platform threads. + */ + AUTO(DefaultValues.DEFAULT_TRANSPORT_THREADPOOL), + /** Review Comment: `AUTO`’s enum `code` is wired to `DefaultValues.DEFAULT_TRANSPORT_THREADPOOL`. Since `code` is part of parsing/serialization (via `from(String)`), changing the default in the future would also silently change the accepted string for `AUTO`. Consider hardcoding the enum’s code to "auto" and keeping the default value separate (only for config defaults). -- 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]
