sergey-chugunov-1985 commented on code in PR #12243:
URL: https://github.com/apache/ignite/pull/12243#discussion_r2278470154
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/wal/reader/StandaloneNoopDiscoverySpi.java:
##########
Review Comment:
What do you think about an idea to move this spi implementation to a more
general package (maybe even some utility one)?
Though I understand why this class sits somewhere in persistence package
tree, it still feels weird to me to see a discovery implementation in that tree.
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/isolated/IsolatedDiscoverySpi.java:
##########
@@ -129,6 +130,11 @@ public class IsolatedDiscoverySpi extends IgniteSpiAdapter
implements IgniteDisc
locNode = new IsolatedNode(ignite.configuration().getNodeId(), attrs,
ver);
}
+ /** {@inheritDoc} */
+ @Override public void setMessageFactory(MessageFactory msgFactory) {
+ // Np-op.
Review Comment:
Typo: `Np-op` instead of `No-op`. I believe you'be made this typo when
thinking really hard about P vs NP problem :)
##########
modules/core/src/main/java/org/apache/ignite/spi/discovery/tcp/TcpDiscoverySpi.java:
##########
@@ -2817,4 +2835,167 @@ private class TcpDiscoverySpiMBeanImpl extends
IgniteSpiMBeanAdapter implements
impl.dumpRingStructure(log);
}
}
+
+ /** Thread-local session for message input/output operations. */
+ private static class MessageIoSession {
+ /** */
+ private static final byte[] JAVA_SERIALIZATION = new byte[] { 0 };
+
+ /** */
+ private static final byte[] MESSAGE_SERIALIZATION = new byte[] { 1 };
+
+ /** */
+ private static final ThreadLocal<MessageIoSession> locSes = new
ThreadLocal<>();
Review Comment:
Approach with ThreadLocal holding `MessageIoSession` objects looks confusing
to me: it makes less obvious what this code is doing. I believe better way to
express this logic is to create a client-like object which would be responsible
for sending and receiving messages, allocating buffers and making
connection-related checks like an SSL checks.
Could you please create a ticket and add it as a TODO here?
##########
modules/core/src/main/java/org/apache/ignite/internal/managers/discovery/GridDiscoveryManager.java:
##########
@@ -345,6 +345,7 @@ public GridDiscoveryManager(GridKernalContext ctx) {
DiscoverySpi spi = getSpi();
spi.setNodeAttributes(ctx.nodeAttributes(), VER);
+ spi.setMessageFactory(ctx.io().messageFactory());
Review Comment:
For now it is okay I guess to reuse existing `messageFactory`, but for the
future I would split discovery messages factory into a separate entity. I don't
know why but I don't like the idea of mixing two sets of messages in one place.
One reason for that is that a separate message factory should provide you with
some kind of a separate namespace for messages and enables you to pick new
messages' directTypes independently from communication messages without risks
of mixing two types together.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
