-
Notifications
You must be signed in to change notification settings - Fork 818
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce H2 ValueType to serialize PublishedMessage #592
Conversation
The build failure on ubuntu-latest is fixed by PR #581 |
|
||
@Override | ||
public int getMemory(Object obj) { | ||
final SessionRegistry.PublishedMessage casted = (SessionRegistry.PublishedMessage) obj; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of casting we could also delegate to the different implementations, to avoid a CCE when a PubRelMarker is persisted.
It may even be an idea to have EnqueuedMessage implement DataType directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid a CCE when a PubRelMarker is
I think to have a type check for the admitted serializable elements, adding for example PubRelMarker
EnqueuedMessage implement DataType
DataType is a descriptor of the data to encode, like a codec, so it's not correct that the data to be encoded implement the codec itself, else the class go against the Single Responsibility Principle. I prefer to keep them separated
5b8491f
to
89c2b56
Compare
Hi @hylkevds may I ask you to give an eye to this PR to know if it's good for you? |
final String token = casted.getTopic().toString(); | ||
topicDataType.write(buff, token); | ||
|
||
final int payloadSize = casted.getPayload().readableBytes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a totally clean implementation, it'd be nice to wrap this (and the corresponding part of read()) in a ByteBufDataType, just like the StringDataType used for the topic string. But since it's currently only used here not a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for suggestion, introduced
broker/src/main/java/io/moquette/persistence/EnqueuedMessageValueType.java
Show resolved
Hide resolved
Looks good, just noticed two things that don't actually influence the functioning of the code. |
…PubRelMarker and PublishedMessage)
Introduce H2 ValueType and use it in Builder to serialize PublishedMessage instances.
Fixes #566
Closes #568