IGNITE-26111 TcpDiscoverySpi uses MessageSerializer#12243
IGNITE-26111 TcpDiscoverySpi uses MessageSerializer#12243timoninmaxim merged 12 commits intoapache:masterfrom
Conversation
timoninmaxim
force-pushed
the
IGNITE-26111__discovery_serialization
branch
6 times, most recently
from
468d8ce to
edd7a32
Compare
|
There was a problem hiding this 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.
| private static final byte[] MESSAGE_SERIALIZATION = new byte[] { 1 }; | ||
|
|
||
| /** */ | ||
| private static final ThreadLocal<MessageIoSession> locSes = new ThreadLocal<>(); |
There was a problem hiding this 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?
|
|
||
| /** {@inheritDoc} */ | ||
| @Override public void setMessageFactory(MessageFactory msgFactory) { | ||
| // Np-op. |
There was a problem hiding this comment.
Typo: Np-op instead of No-op. I believe you'be made this typo when thinking really hard about P vs NP problem :)
| DiscoverySpi spi = getSpi(); | ||
|
|
||
| spi.setNodeAttributes(ctx.nodeAttributes(), VER); | ||
| spi.setMessageFactory(ctx.io().messageFactory()); |
There was a problem hiding this 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.
There was a problem hiding this comment.
Will create a separate MessageFactory for discovery
timoninmaxim
force-pushed
the
IGNITE-26111__discovery_serialization
branch
from
edd7a32 to
3e81a15
Compare
…discovery_serialization
timoninmaxim
force-pushed
the
IGNITE-26111__discovery_serialization
branch
from
bcd3ac0 to
016f4a3
Compare
|
2 participants
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see TC.Bot: Check PR)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.