Add publish header "Nats-Expected-Last-Subject-Sequence-Subject" #1401
Conversation
| if (pubStream != null && !pubStream.equals(ackStream)) { | ||
| throw new IOException("Expected ack from stream " + pubStream + ", received from: " + ackStream); | ||
| if (optAckStream != null && !optAckStream.equals(ackStream)) { | ||
| throw new IOException("Expected ack from stream " + optAckStream + ", received from: " + ackStream); |
There was a problem hiding this comment.
While I was fixing this code I came across the PublishOptions.getStream() and was confused because there is also getExpectedStream() So I made the name clearer.
| * @deprecated Just use null to unset | ||
| * Use this variable to unset a stream in publish options. | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
The constant is null. Sigh, I blame myself.
| public static final long UNSET_LAST_SEQUENCE = -1; | ||
|
|
||
| private final String stream; | ||
| private final String pubAckStream; |
There was a problem hiding this comment.
rename / clarification
| private final Duration streamTimeout; | ||
| private final String expectedStream; | ||
| private final String expectedLastId; | ||
| private final String expectedLastMsgId; |
There was a problem hiding this comment.
better name for this variable too, also matches the getter
| * Gets the name of the stream. | ||
| * @return the name of the stream. | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
This is an internal class, but is public so am deprecating
| this.expectedLastMsgId = b.expectedLastMsgId; | ||
| this.expectedLastSeq = b.expectedLastSeq; | ||
| this.expectedLastSubSeq = b.expectedLastSubSeq; | ||
| this.expectedLastSubSeqSubject = b.expectedLastSubSeqSubject; |
| assertTrue(ee.getCause() instanceof RuntimeException); | ||
| assertTrue(ee.getCause().getCause() instanceof IOException); | ||
| assertInstanceOf(RuntimeException.class, ee.getCause()); | ||
| assertInstanceOf(IOException.class, ee.getCause().getCause()); |
There was a problem hiding this comment.
just cleaning up stuff since I'm already in the test
| * @param stream The name of the stream. | ||
| * @return The Builder | ||
| */ | ||
| public Builder pubAckStream(String stream) { |
There was a problem hiding this comment.
Should this behavior be deprecated maybe?
My understanding is that the server supports Nats-Expected-Stream, and that's the intended way to not persist the message into the stream unless it's what you expect. This is already supported with expectedStream(stream).
stream/pubAckStream will let you persist data in the stream you don't expect, and then error once the PubAck reaches the client?
There was a problem hiding this comment.
I have no idea what the history of this is. Deprecating seems like a good idea since it's pretty worthless to check the expectation after the publish.
There was a problem hiding this comment.
I'm also removing the new naming. No point adding anew name if it's just deprecated. Also changing the comment as to why it's deprecated.
| expectedLastSubSeq = UNSET_LAST_SEQUENCE; | ||
| expectedLastSubSeqSubject = null; | ||
| msgId = null; | ||
| messageTtl = null; |
There was a problem hiding this comment.
Was this intentional? The messageTtl shouldn't be an "expected-type" check?
There was a problem hiding this comment.
So the question is if they are re-using the builder, what fields should be kept. Would they want to continue using the messageTtl on every message for the user? Now that I write it out, maybe they would want the same ttl on every message.
This is just a convenience method anyway as they could always create a new instance for every message. And if it is re-used they can always set over it anyway.
But I'm going to remove clearing messageTtl and update the comment
|
|
||
| /** | ||
| * @deprecated Prefer pubAckStream(...) | ||
| * @deprecated Not a very useful function |
There was a problem hiding this comment.
Should this suggest to migrate to expectedStream(..) instead?
No description provided.