Skip to content
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

Add Direct Get by start time #1226

Merged
merged 6 commits into from
Sep 23, 2024
Merged

Add Direct Get by start time #1226

merged 6 commits into from
Sep 23, 2024

Conversation

MauriceVanVeen
Copy link
Member

No description provided.

Signed-off-by: Maurice van Veen <[email protected]>
* server such as timeout or interruption
* @throws JetStreamApiException the request had an error related to the data
*/
MessageInfo getFirstMessage(String streamName, ZonedDateTime startTime) throws IOException, JetStreamApiException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there ever a case when we will want first message by subject and start time? It's probably fine to have this since it seems like a common behavior, but I think it's time to expose something like

MessageInfo getMessage(String streamName, MessageGetRequest messageGetRequest) 

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, in combination with the new builder.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some discussion offline removed the builder again, as the only combinations that needed to be added were:

  • getFirstMessage(streamName, startTime)
  • getFirstMessage(streamName, startTime, subject)

And then we don't need extra validation in the builder for valid combinations.

}

private MessageGetRequest(long sequence, String lastBySubject, String nextBySubject) {
private MessageGetRequest(long sequence, String lastBySubject, String nextBySubject, ZonedDateTime startTime) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should add a builder to this class, especially if we are going to add a generic get message api

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the builder 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -1305,6 +1306,7 @@ private void validateGetMessage(JetStreamManagement jsm, TestingStreamContainer

assertStatus(10003, assertThrows(JetStreamApiException.class, () -> jsm.getMessage(tsc.stream, -1)));
assertStatus(10003, assertThrows(JetStreamApiException.class, () -> jsm.getMessage(tsc.stream, 0)));
assertStatus(10003, assertThrows(JetStreamApiException.class, () -> jsm.getFirstMessage(tsc.stream, DEFAULT_TIME)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, is it correct that this fails because the start time is before the very first message in the stream?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't fail because of the start time being before the first message.
Instead it fails because a DEFAULT_TIME doesn't get serialized into the final JSON, resulting in an empty JSON object being sent which is checked here should be an error.

Signed-off-by: Maurice van Veen <[email protected]>
@MauriceVanVeen MauriceVanVeen marked this pull request as draft September 17, 2024 17:28
@MauriceVanVeen
Copy link
Member Author

Drafting for now, needs a little more polish to capture the valid combinations in MessageGetRequest.

@MauriceVanVeen MauriceVanVeen marked this pull request as ready for review September 17, 2024 19:41
Copy link
Contributor

@scottf scottf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Maurice van Veen <[email protected]>
Copy link
Contributor

@scottf scottf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MauriceVanVeen MauriceVanVeen merged commit 95987a2 into main Sep 23, 2024
4 checks passed
@MauriceVanVeen MauriceVanVeen deleted the direct-get-by-start-time branch September 23, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants