Copilot commented on code in PR #13215:
URL: https://github.com/apache/ignite/pull/13215#discussion_r3363652640
##########
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) {
+ if (opCtx.skipReadThrough())
+ return this;
+ else
+ opCtx = opCtx.withSkipReadThrough();
Review Comment:
`withSkipReadThrough()` mutates the proxy field `opCtx` (assigning to
`opCtx` inside the method). This makes projections stateful and can leak flags
into the original proxy instance; compute a local `newOpCtx` instead and leave
`this.opCtx` unchanged.
##########
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) {
+ if (opCtx.skipStore())
+ return this;
+ else
+ opCtx = opCtx.withSkipStore();
Review Comment:
`withSkipStore()` mutates the proxy field `opCtx` (via assignments like
`opCtx = opCtx.withSkipStore()`), which makes cache projections stateful and
can unexpectedly change behavior of the original proxy instance (and is unsafe
if the proxy is shared across threads). This method should compute a new local
context and return a new proxy without modifying `this.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
+ opCtx = opCtx.withKeepBinary();
Review Comment:
`keepBinary()` mutates the proxy field `opCtx` and then returns a new proxy.
Projection methods should not modify the existing proxy instance (it can be
shared and is expected to be immutable-like). Use a local `newOpCtx` when
deriving the new projection.
##########
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);
+ else
+ opCtx =
CacheOperationContext.builder().expiryPolicy(plc).build();
+
+ return new GridCacheProxyImpl<>(ctx, delegate, opCtx);
Review Comment:
`withExpiryPolicy()` assigns to the proxy field `opCtx` inside the method.
This makes projections stateful and can leak the expiry policy into the
original proxy instance. Derive a local `newOpCtx` and pass it to the new proxy
without mutating `this.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);
Review Comment:
`withApplicationAttributes()` assigns to the proxy field `opCtx`, which can
unexpectedly modify the current projection (and any other code holding a
reference to it). This method should build a new local context and return a new
proxy without mutating `this.opCtx`.
##########
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())
+ return this;
+ else
+ opCtx = opCtx.withCalciteEngine();
+ }
+ else
+ opCtx =
CacheOperationContext.builder().calciteEngine(true).build();
+
+ return new GridCacheProxyImpl<>(ctx, delegate, opCtx);
Review Comment:
`withCalciteEngine()` mutates the proxy field `opCtx` while building a new
projection. Projection-creation methods should not change the current proxy
instance; compute a local `newOpCtx` and return a new proxy built from it.
##########
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);
Review Comment:
`withNoRetries()` mutates the proxy field `opCtx` (assigning to it inside
the method). This can unintentionally change flags on the current projection
and is unsafe if the proxy is shared. Use a local `newOpCtx` and keep
`this.opCtx` unchanged.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/IgniteInternalCache.java:
##########
@@ -270,7 +269,7 @@ public interface IgniteInternalCache<K, V> extends
Iterable<Cache.Entry<K, V>> {
*
* @return New internal cache instance for binary objects.
*/
- public <K1, V1> IgniteInternalCache<K1, V1> keepBinary();
+ public IgniteInternalCache<K, V> keepBinary();
Review Comment:
`keepBinary()` return type was changed to `IgniteInternalCache<K, V>`, but
the Javadoc for this method describes (and demonstrates) obtaining a projection
with different generic value type for binary objects. This signature change
removes that type-safety and makes the current Javadoc/example misleading;
please either restore the generic signature or update the documentation/usages
accordingly.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/atomic/GridNearAtomicSingleUpdateFuture.java:
##########
@@ -100,7 +101,8 @@ public GridNearAtomicSingleUpdateFuture(
boolean keepBinary,
boolean recovery,
int remapCnt,
- @Nullable Map<String, String> appAttrs
+ @Nullable Map<String, String> appAttrs,
+ boolean unwrap
) {
Review Comment:
The constructor parameter name `unwrap` is misleading: it is passed to
`GridNearAtomicAbstractUpdateFuture` as the Calcite-engine call flag. Please
rename it to reflect its meaning (and keep naming consistent with
`GridNearAtomicUpdateFuture`).
--
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]
