anton-vinogradov commented on code in PR #13095:
URL: https://github.com/apache/ignite/pull/13095#discussion_r3596216694
##########
modules/nio/src/main/java/org/apache/ignite/plugin/extensions/communication/Message.java:
##########
@@ -32,19 +48,25 @@ public interface Message {
/** Registry of message class to direct type mappings, populated during
factory initialization. */
Map<Class<?>, Short> REGISTRATIONS = new ConcurrentHashMap<>();
Review Comment:
Done — `Map<Class<? extends Message>, Short>` (in the default method
`getClass()` is statically `Class<? extends Message>`, so `put` needs no cast).
##########
modules/nio/src/main/java/org/apache/ignite/plugin/extensions/communication/Message.java:
##########
@@ -23,7 +23,23 @@
import
org.apache.ignite.internal.managers.communication.UnknownMessageException;
/**
- * Base class for all communication messages.
+ * Base type for all messages sent between nodes, both over the communication
SPI and via discovery.
+ * <p>
+ * Serialized fields are declared by annotating instance fields; {@ignitelink
org.apache.ignite.internal.MessageProcessor} then
Review Comment:
`Message` lives in the `nio` module, while `MessageProcessor` / `@Order` /
`@Marshalled` live in `codegen`, and `nio` doesn't depend on it — a regular
`{@link}` can't resolve there and fails the javadoc build. `{@ignitelink}` is
the project's taglet (`IgniteLinkTaglet` in `modules/tools`) made exactly for
such cross-module references.
##########
modules/nio/src/main/java/org/apache/ignite/plugin/extensions/communication/Message.java:
##########
Review Comment:
The registry isn't serialization — it's the message's wire identity: on
master `directType()` is `Message`'s own method, each class hard-coding its id.
The registry just replaced those per-class overrides with a map behind the same
`Message`-level contract, so it stays where the contract lives, next to its
only reader (`directType()`/`DIRECT_TYPES`). A serializer knows how to write a
message; the type id is a property of the message itself.
##########
modules/nio/src/main/java/org/apache/ignite/plugin/extensions/communication/Message.java:
##########
@@ -32,19 +48,25 @@ public interface Message {
/** Registry of message class to direct type mappings, populated during
factory initialization. */
Map<Class<?>, Short> REGISTRATIONS = new ConcurrentHashMap<>();
+ /** Per-class cache over {@link #REGISTRATIONS}; keeps {@link
#directType()} off the map lookup done for every sent message. */
+ ClassValue<Short> DIRECT_TYPES = new ClassValue<>() {
Review Comment:
The `ClassValue` isn't a rename, it's a cache with a contract: a plain
`REGISTRATIONS.get(getClass())` returns `null` for an unregistered class → NPE
on unboxing, while `DIRECT_TYPES` throws `UnknownMessageException`, which
production discovery (`TcpDiscoverySpi`, `ServerImpl`, `TcpDiscoveryIoSession`)
catches for unknown-type tolerance and `DiscoverySerializationExceptionTest`
covers.
##########
modules/nio/src/main/java/org/apache/ignite/plugin/extensions/communication/Message.java:
##########
@@ -32,19 +48,25 @@ public interface Message {
/** Registry of message class to direct type mappings, populated during
factory initialization. */
Map<Class<?>, Short> REGISTRATIONS = new ConcurrentHashMap<>();
+ /** Per-class cache over {@link #REGISTRATIONS}; keeps {@link
#directType()} off the map lookup done for every sent message. */
+ ClassValue<Short> DIRECT_TYPES = new ClassValue<>() {
+ @Override protected Short computeValue(Class<?> type) {
+ Short directType = REGISTRATIONS.get(type);
+
+ if (directType == null)
+ throw new
UnknownMessageException(type.asSubclass(Message.class));
+
+ return directType;
+ }
+ };
+
/**
* Gets message type.
*
* @return Message type.
*/
default short directType() {
- var clazz = getClass();
- Short type = REGISTRATIONS.get(clazz);
-
- if (type == null)
- throw new UnknownMessageException(clazz);
-
- return type;
+ return DIRECT_TYPES.get(getClass());
Review Comment:
That would drop both things `DIRECT_TYPES` provides: the
`UnknownMessageException` contract (a bare `get` NPEs on unboxing for an
unregistered class; discovery catches that exception in
`TcpDiscoverySpi`/`ServerImpl`/`TcpDiscoveryIoSession`, covered by
`DiscoverySerializationExceptionTest`) and the per-class cache — `directType()`
runs for every message written, and `ClassValue` is the JDK mechanism for
exactly this hot-path per-class lookup.
--
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]
