Copilot commented on code in PR #7611: URL: https://github.com/apache/ignite-3/pull/7611#discussion_r2918458918
########## modules/runner/src/main/java/org/apache/ignite/internal/app/JobScopedIgnite.java: ########## @@ -0,0 +1,103 @@ +/* + * 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.app; + +import java.util.concurrent.Executor; +import org.apache.ignite.Ignite; +import org.apache.ignite.catalog.IgniteCatalog; +import org.apache.ignite.compute.IgniteCompute; +import org.apache.ignite.internal.hlc.HybridTimestampTracker; +import org.apache.ignite.internal.sql.api.IgniteSqlImpl; +import org.apache.ignite.internal.sql.api.JobScopedIgniteSql; +import org.apache.ignite.internal.sql.api.PublicApiThreadingIgniteSql; +import org.apache.ignite.internal.tx.TxManager; +import org.apache.ignite.internal.tx.impl.IgniteTransactionsImpl; +import org.apache.ignite.internal.tx.impl.PublicApiThreadingIgniteTransactions; +import org.apache.ignite.internal.wrapper.Wrapper; +import org.apache.ignite.network.IgniteCluster; +import org.apache.ignite.sql.IgniteSql; +import org.apache.ignite.table.IgniteTables; +import org.apache.ignite.tx.IgniteTransactions; + +/** + * A lightweight wrapper around {@link IgniteImpl} that scopes {@link #transactions()} and {@link #sql()} to a per-job + * {@link HybridTimestampTracker}. This prevents one compute job from polluting another job's observable timestamp. + */ +class JobScopedIgnite implements Ignite, Wrapper { + private final Ignite delegate; + + private final IgniteTransactions scopedTransactions; + + private final IgniteSql scopedSql; + + JobScopedIgnite( + Ignite delegate, + HybridTimestampTracker jobTracker, + TxManager txManager, + IgniteSqlImpl sql, + Executor asyncContinuationExecutor + ) { + this.delegate = delegate; + this.scopedTransactions = new PublicApiThreadingIgniteTransactions( + new IgniteTransactionsImpl(txManager, jobTracker), asyncContinuationExecutor + ); + this.scopedSql = new PublicApiThreadingIgniteSql( + new JobScopedIgniteSql(sql, jobTracker), asyncContinuationExecutor + ); + } + + @Override + public String name() { + return delegate.name(); + } + + @Override + public IgniteTables tables() { + return delegate.tables(); + } + + @Override + public IgniteTransactions transactions() { + return scopedTransactions; + } + + @Override + public IgniteSql sql() { + return scopedSql; + } + + @Override + public IgniteCompute compute() { + return delegate.compute(); + } + + @Override + public IgniteCatalog catalog() { + return delegate.catalog(); + } + + @Override + public IgniteCluster cluster() { + return delegate.cluster(); + } + + @Override + public <T> T unwrap(Class<T> classToUnwrap) { Review Comment: `JobScopedIgnite.unwrap` always casts the direct delegate and does not attempt wrapper chaining (e.g., if `delegate` itself is a `Wrapper`) and also can’t unwrap to `JobScopedIgnite` itself. Consider implementing `unwrap` in a wrapper-friendly way (check `this` first, then delegate via `Wrapper#unwrap` when applicable, otherwise cast). ```suggestion public <T> T unwrap(Class<T> classToUnwrap) { if (classToUnwrap.isInstance(this)) { return classToUnwrap.cast(this); } if (delegate instanceof Wrapper) { return ((Wrapper) delegate).unwrap(classToUnwrap); } ``` ########## modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java: ########## @@ -325,6 +325,8 @@ public class IgniteImpl implements Ignite { private final Path workDir; + private Executor asyncContinuationExecutor; + Review Comment: `asyncContinuationExecutor` is assigned once in the constructor and then used to create per-job wrappers; making it `final` (and preferably `@NotNull`-validated) would better communicate immutability and avoid accidental reassignment later. ########## modules/runner/src/main/java/org/apache/ignite/internal/app/JobScopedIgnite.java: ########## @@ -0,0 +1,103 @@ +/* + * 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.app; + +import java.util.concurrent.Executor; +import org.apache.ignite.Ignite; +import org.apache.ignite.catalog.IgniteCatalog; +import org.apache.ignite.compute.IgniteCompute; +import org.apache.ignite.internal.hlc.HybridTimestampTracker; +import org.apache.ignite.internal.sql.api.IgniteSqlImpl; +import org.apache.ignite.internal.sql.api.JobScopedIgniteSql; +import org.apache.ignite.internal.sql.api.PublicApiThreadingIgniteSql; +import org.apache.ignite.internal.tx.TxManager; +import org.apache.ignite.internal.tx.impl.IgniteTransactionsImpl; +import org.apache.ignite.internal.tx.impl.PublicApiThreadingIgniteTransactions; +import org.apache.ignite.internal.wrapper.Wrapper; +import org.apache.ignite.network.IgniteCluster; +import org.apache.ignite.sql.IgniteSql; +import org.apache.ignite.table.IgniteTables; +import org.apache.ignite.tx.IgniteTransactions; + +/** + * A lightweight wrapper around {@link IgniteImpl} that scopes {@link #transactions()} and {@link #sql()} to a per-job + * {@link HybridTimestampTracker}. This prevents one compute job from polluting another job's observable timestamp. + */ +class JobScopedIgnite implements Ignite, Wrapper { + private final Ignite delegate; + + private final IgniteTransactions scopedTransactions; + + private final IgniteSql scopedSql; + + JobScopedIgnite( + Ignite delegate, + HybridTimestampTracker jobTracker, + TxManager txManager, + IgniteSqlImpl sql, + Executor asyncContinuationExecutor + ) { + this.delegate = delegate; + this.scopedTransactions = new PublicApiThreadingIgniteTransactions( + new IgniteTransactionsImpl(txManager, jobTracker), asyncContinuationExecutor + ); + this.scopedSql = new PublicApiThreadingIgniteSql( + new JobScopedIgniteSql(sql, jobTracker), asyncContinuationExecutor + ); + } + + @Override + public String name() { + return delegate.name(); + } + + @Override + public IgniteTables tables() { + return delegate.tables(); + } Review Comment: `JobScopedIgnite.tables()` delegates directly to the node-scoped `IgniteTables`. Table operations executed with `null` transaction start implicit transactions inside `InternalTableImpl` using its own `observableTimestampTracker` (node-global), so compute jobs using Table API can still update/pollute the node-global tracker and won’t use the per-job observable timestamp. Consider also scoping `tables()` (e.g., a job-scoped tables/view wrapper that passes the per-job tracker into implicit tx creation), or clearly enforcing/ensuring table operations in jobs always use an explicit transaction from `transactions()`. -- 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]
