nizhikov commented on PR #13647: URL: https://github.com/apache/ignite/pull/13647#issuecomment-5994293942
## Review: removing `CacheObjectValueContext#classLoader()` **Verdict: correct.** The removed accessor and `BinaryContext#classLoader()` always return the same value, so replacing one with the other in `BinaryObjectImpl.deserializeValue` changes no behaviour. ### Why the values are identical - `AbstractCacheObjectContext.classLoader()` returned `ctx.config().getClassLoader()`, i.e. the kernal context's `IgniteConfiguration`. - The node's `BinaryContext` is created in `CacheObjectBinaryProcessorImpl` (line 252) via `U.binaryContext(metaHnd, marsh, ctx.config(), log)`, and that wrapper passes `cfg.getClassLoader()` straight into the `BinaryContext` constructor, which stores it unchanged and returns it from `classLoader()`. Same configuration instance, same getter, same value — including the `null` case. - No other pairing can occur: `CacheObjectValueContext` has three implementations (`CacheObjectContext`, `AbstractCacheObjectContext`, `CacheQueryObjectValueContext`), all in core and all built on a kernal context. The `BinaryContext` instances created by the thin client, JDBC and platform code are never combined with a `CacheObjectValueContext`. `IgniteMock` in tests also builds its context from `cfg.getClassLoader()`. ### Null path When no class loader is configured, the old code passed `null` into `reader(rCtx, ldr, forUnmarshal)`, which falls back to `ctx.classLoader()` (also `null`) at `BinaryObjectImpl.java:816`. Downstream resolution is therefore identical before and after. ### `storeValue` relocation Moving the `storeValue()` check from `deserializeValue(coCtx)` into `value()` is equivalent: `value()` was the only caller with a non-null context, and `deserialize()` passed `null`, so it never stored and still does not. ### Verification - `ignite-binary-api`, `ignite-binary-impl` compile; `ignite-core` compiles including test sources, so no caller of the removed method remains. - No file referencing `CacheObjectValueContext` in any module calls `classLoader()` on anything other than a `BinaryContext`; `BinaryObjectOffheapImpl` was already using `ctx.classLoader()` only. ### Nit (readability only) In `value(CacheObjectValueContext ctx, boolean cpy)` the parameter `ctx` shadows the `BinaryContext` field of the same name, so `ctx.storeValue()` in that method is the parameter while `ctx.classLoader()` in `deserializeValue()` is the field. It compiles to the right thing; renaming the parameter to `coCtx` would make the diff self-evident. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
