Copilot commented on code in PR #13215:
URL: https://github.com/apache/ignite/pull/13215#discussion_r3371095117
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProxyImpl.java:
##########
@@ -281,21 +276,16 @@ public IgniteInternalCache<K, V> delegate() {
CacheOperationContext prev = gate.enter(opCtx);
try {
- if (opCtx != null && opCtx.skipReadThrough())
- return this;
+ if (opCtx != null) {
Review Comment:
`withSkipReadThrough()` also mutates the proxy’s `opCtx` field. This changes
the behavior of the existing cache proxy instance and can introduce
cross-thread races; the method should be side-effect free and only affect the
returned projection.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProxyImpl.java:
##########
@@ -307,41 +297,30 @@ public IgniteInternalCache<K, V> delegate() {
CacheOperationContext prev = gate.enter(opCtx);
try {
- return new GridCacheProxyImpl<>(ctx, delegate,
- opCtx != null ? opCtx.setApplicationAttributes(attrs) :
- new CacheOperationContext(
- false,
- false,
- false,
- null,
- false,
- null,
- false,
- null,
- attrs));
+ if (opCtx != null)
Review Comment:
`withApplicationAttributes()` currently assigns to the proxy field `opCtx`.
This makes the current projection stateful and can leak application attributes
into subsequent operations on the same proxy. Use a local `newOpCtx` and keep
the original proxy unchanged.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProxyImpl.java:
##########
@@ -1604,17 +1577,37 @@ public IgniteInternalCache<K, V> delegate() {
CacheOperationContext prev = gate.enter(opCtx);
try {
- return new GridCacheProxyImpl<>(ctx, delegate,
- new CacheOperationContext(
- false,
- false,
- false,
- null,
- true,
- null,
- false,
- null,
- null));
+ if (opCtx != null) {
+ if (opCtx.noRetries())
Review Comment:
`withNoRetries()` mutates the proxy field `opCtx`, which can unexpectedly
change the behavior of the current projection and introduces potential
cross-thread races. Compute the updated context in a local variable and keep
`this` immutable.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProxyImpl.java:
##########
@@ -1604,17 +1577,37 @@ public IgniteInternalCache<K, V> delegate() {
CacheOperationContext prev = gate.enter(opCtx);
try {
- return new GridCacheProxyImpl<>(ctx, delegate,
- new CacheOperationContext(
- false,
- false,
- false,
- null,
- true,
- null,
- false,
- null,
- null));
+ if (opCtx != null) {
+ if (opCtx.noRetries())
+ return this;
+ else
+ opCtx = opCtx.withNoRetries();
+ }
+ else
+ opCtx =
CacheOperationContext.builder().noRetries(true).build();
+
+ return new GridCacheProxyImpl<>(ctx, delegate, opCtx);
+ }
+ finally {
+ gate.leave(prev);
+ }
+ }
+
+ /** {@inheritDoc} */
+ @Override public IgniteInternalCache<K, V> withCalciteEngine() {
+ CacheOperationContext prev = gate.enter(opCtx);
+
+ try {
+ if (opCtx != null) {
+ if (opCtx.calciteEngine())
Review Comment:
`withCalciteEngine()` mutates the proxy field `opCtx`. Like other projection
methods, this should be side-effect free and return a new proxy with the
updated context without altering the current instance.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProxyImpl.java:
##########
@@ -251,25 +251,20 @@ public IgniteInternalCache<K, V> delegate() {
}
/** {@inheritDoc} */
- @Override public GridCacheProxyImpl<K, V> setSkipStore(boolean skipStore) {
+ @Override public GridCacheProxyImpl<K, V> withSkipStore() {
CacheOperationContext prev = gate.enter(opCtx);
try {
- if (opCtx != null && opCtx.skipStore() == skipStore)
- return this;
+ if (opCtx != null) {
Review Comment:
`withSkipStore()` mutates the proxy’s `opCtx` field before returning a new
projection. Cache projections are expected to be immutable (the original proxy
should not change), and mutating shared state here can lead to surprising
behavior and thread-safety issues. Compute the new context in a local variable
instead of assigning to `opCtx`.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProxyImpl.java:
##########
@@ -307,41 +297,30 @@ public IgniteInternalCache<K, V> delegate() {
CacheOperationContext prev = gate.enter(opCtx);
try {
- return new GridCacheProxyImpl<>(ctx, delegate,
- opCtx != null ? opCtx.setApplicationAttributes(attrs) :
- new CacheOperationContext(
- false,
- false,
- false,
- null,
- false,
- null,
- false,
- null,
- attrs));
+ if (opCtx != null)
+ opCtx = opCtx.withApplicationAttributes(attrs);
+ else
+ opCtx =
CacheOperationContext.builder().applicationAttributes(attrs).build();
+
+ return new GridCacheProxyImpl<>(ctx, delegate, opCtx);
}
finally {
gate.leave(prev);
}
}
/** {@inheritDoc} */
- @Override public <K1, V1> GridCacheProxyImpl<K1, V1> keepBinary() {
- if (opCtx != null && opCtx.isKeepBinary())
- return (GridCacheProxyImpl<K1, V1>)this;
+ @Override public GridCacheProxyImpl<K, V> keepBinary() {
+ if (opCtx != null) {
+ if (opCtx.isKeepBinary())
+ return this;
+ else
Review Comment:
`keepBinary()` no longer matches the generic method signature from
`IgniteInternalCache` (`<K1,V1> IgniteInternalCache<K1,V1> keepBinary()`),
which will cause a compilation error (“name clash”/does not implement method).
It also mutates the proxy’s `opCtx` field; compute the new context locally and
keep the original projection unchanged.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/GridDhtLockFuture.java:
##########
@@ -203,6 +206,8 @@ public final class GridDhtLockFuture extends
GridCacheCompoundIdentityFuture<Boo
* @param threadId Thread ID.
* @param accessTtl TTL for read operation.
* @param skipStore Skip store flag.
+ * @param skipReadThrough Skip read-through cache store flag.
+ * @param calciteOpCall Calcite engine operation call.
*/
Review Comment:
The constructor Javadoc is missing the `@param keepBinary` entry even though
`keepBinary` is a constructor parameter. Please document it to keep the Javadoc
accurate.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/GridCacheProxyImpl.java:
##########
@@ -1581,18 +1560,12 @@ public IgniteInternalCache<K, V> delegate() {
CacheOperationContext prev = gate.enter(opCtx);
try {
- return new GridCacheProxyImpl<>(ctx, delegate,
- opCtx != null ? opCtx.withExpiryPolicy(plc) :
- new CacheOperationContext(
- false,
- false,
- false,
- plc,
- false,
- null,
- false,
- null,
- null));
+ if (opCtx != null)
+ opCtx = opCtx.withExpiryPolicy(plc);
Review Comment:
`withExpiryPolicy()` mutates the proxy’s `opCtx` field; this makes the
existing cache projection stateful and can leak the expiry policy into later
calls on the same proxy instance. Use a local `newOpCtx` and return a new proxy
without changing `this.opCtx`.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/atomic/GridNearAtomicSingleUpdateFuture.java:
##########
@@ -82,6 +83,7 @@ public class GridNearAtomicSingleUpdateFuture extends
GridNearAtomicAbstractUpda
* @param keepBinary Keep binary flag.
* @param recovery {@code True} if cache operation is called in recovery
mode.
* @param remapCnt Maximum number of retries.
+ * @param calciteOpCall Calcite engine opearation call.
Review Comment:
Typo in Javadoc: "opearation" -> "operation".
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearLockFuture.java:
##########
@@ -185,6 +188,8 @@ public final class GridNearLockFuture extends
GridCacheCompoundIdentityFuture<Bo
* @param skipStore skipStore
* @param keepBinary Keep binary flag.
* @param recovery Recovery flag.
+ * @param skipReadThrough Skip read-through cache store flag.
+ * @param calciteOpCall Calcite engine operation call.
Review Comment:
The constructor Javadoc lists parameters out of order (it documents
`keepBinary`/`recovery` before the newly added
`skipReadThrough`/`calciteOpCall`). This makes the Javadoc misleading when
reading the signature.
--
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]
