Simplification Review 1 #917

scottf · 2023-05-23T01:04:29Z

No description provided.

…better exception handling comments

MauriceVanVeen · 2023-05-24T17:56:49Z

src/main/java/io/nats/client/impl/NatsConnection.java

@@ -2128,6 +2128,20 @@ void lenientFlushBuffer()  {
        }
    }

+    @Override
+    public StreamContext streamContext(String streamName) throws IOException, JetStreamApiException {


When looking at an example, it would become:

streamContext = nc.streamContext(STREAM); streamContext.addConsumer(ConsumerConfiguration.builder().durable(CONSUMER_NAME).build()); consumerContext = streamContext.getConsumerContext(CONSUMER_NAME);

Would it be nice to name it to getStreamContext so it's consistent with getConsumerContext?

So maybe, I'll ask the other team members. get is very java, but the existing Connection interface has methods like jetStream and jetStreamManagement so I wanted to stay consistent. Not sure why those where named like that to begin with, again they aren't very java like.

MauriceVanVeen · 2023-05-24T18:00:20Z

src/examples/java/io/nats/examples/jetstream/NatsJsPullSubIterate.java

-        catch (Exception e) {
-            e.printStackTrace();
+        catch (RuntimeException e) {
+            // Synchronous pull calls, including raw calls, fetch, iterate and reader


Really like these docs! Makes it really clear just looking at the examples 👍

I updated the old pull examples so make sure people see the JetStreamStatusException runtime exception

What about the new examples.

ConsumeManuallyCallNext.java

ConsumeWithHandler.java

FetchExample.java

NextExample.java

I think the examples are very clear. Don't have many/any comments on the examples themselves, more so on the API.

MauriceVanVeen · 2023-05-24T18:02:19Z

src/main/java/io/nats/client/SimpleConsumer.java

+     * Messages do not immediately stop
+     * @throws InterruptedException if one is thrown, in order to propagate it up
+     */
+    void stop() throws InterruptedException;


There used to be a drain here. What would be the alternative approach to block and wait for the stop/drain to complete?

I had originally called that drain, but stop was the choice of the design team, trying to distinguished it from the existing model.

Looking at NatsConnection there is still a drain there as well, could maybe be a point for consistency.

Am mostly concerned with being able to know when drain/stop finishes though, to be able to run some other code after a consumer has drained/stopped for example.

I added a hasPending method to the SimpleConsumer

/** * Whether the Simple Consumer has pending messages to be sent from the server. * This does not indicate that messages are coming, just that the server has not fulfilled * the current request. Mostly helpful after stop has been called to check if any more messages * are coming * @return the pending state */ boolean hasPending();

As I read that explanation, it's not quite right. The intention is for the dev to be able to know when there are no more messages coming after stop has been called, mostly for async consumers (those with handlers) as they won't get a null or terminal message.

MauriceVanVeen · 2023-05-24T18:13:39Z

src/examples/java/io/nats/examples/jetstream/simple/FetchExample.java

+        StreamContext streamContext;
+        ConsumerContext consumerContext;
+        try {
+            streamContext = nc.streamContext(STREAM);


Looking at this example more I'm wondering why it's called on nc and not js.

Would having both under js be preferable, or is there a specific reason to have it under nc?

js.getStreamContext(STREAM); js.getConsumerContext(STREAM, consumerName);

There actually is a getStreamContext and getConsumerContext off of js. I probably should make those consistent (remove the get).
Hopefully I'll get more feedback here.

To me it would sound logical to have to convert the nc: NatsConnection to nc.jetStream() first, so you are explicitly in the JetStream context. And only then having access to the js.getStreamContext / js.getConsumerContext.

That is, if you'd still want to make the distinction though. Since it would mean that the simplification is two calls away instead of just one (nc.jetStream().getStreamContext() vs nc.streamContext()). I would consider having it accessible more quickly to be valuable as well.

I would probably lean towards just having js.getStreamContext() (or js.streamContext()) instead of having those methods on both NatsConnection and JetStream. Having it one additional call away does not really make it worse IMO and gives you clean separation - NatsConnction should not be handling streams, consumers etc.

MauriceVanVeen · 2023-05-24T18:20:34Z

src/examples/java/io/nats/examples/jetstream/simple/NextExample.java

+            try {
+                streamContext = nc.streamContext(STREAM);
+                streamContext.addConsumer(ConsumerConfiguration.builder().durable(CONSUMER_NAME).build());
+                consumerContext = streamContext.getConsumerContext(CONSUMER_NAME);


Expected the returned value of streamContext.addConsumer to be a ConsumerContext.

Makes sense it's still a ConsumerInfo, but I would probably like to be able to do something like this:
(not thinking about errors for this example)

ConsumerContext consumerContext = streamContext.getConsumerContext(CONSUMER_NAME); if (consumerContext == null) { consumerContext = streamContext.addConsumer(ConsumerConfiguration.builder().durable(CONSUMER_NAME).build()); }

That would allow for chaining these commands as well:

ConsumerContext consumerContext = js.getStreamContext(STREAM).addConsumer(...);

Yeah, that's a good point, I wrestled with that. I'll check and see what the other client are doing.

piotrpio

Some questions and some nits I came across.

In general the API looks really good!

piotrpio · 2023-05-31T09:51:52Z

src/main/java/io/nats/client/StreamContext.java

+    /**
+     * Management function to creates a consumer on this stream.
+     * @param config the consumer configuration to use.
+     * @return consumer information.


Suggested change

* @return consumer information.

* @return a ConsumerContext object

piotrpio · 2023-05-31T09:55:49Z

src/main/java/io/nats/client/impl/TrackPendingListener.java

@@ -0,0 +1,18 @@
+// Copyright 2021 The NATS Authors


Suggested change

// Copyright 2021 The NATS Authors

// Copyright 2023 The NATS Authors

piotrpio · 2023-05-31T10:04:02Z

src/main/java/io/nats/client/Connection.java

+     * @throws IOException covers various communication issues with the NATS
+     *         server such as timeout or interruption
+     */
+    StreamContext streamContext(String streamName) throws IOException, JetStreamApiException;


I am not sure it's valuable to have stream/consumer methods on Connection - they seem to just clutter the interface, or is there any clear benefit?

@piotrpio My thought was I don't need a JetStreamContext unless I'm publishing, so it's less steps to get it from the connection, so hanging off connection is my first choice and hanging off JetStream is the second choice. Especially since StreamContext has a lot of the same stuff as JetStreamManagement, it made sense to have it at connection level.
But I was never really settled on it either way, so I guess let's continue to think about this.

piotrpio · 2023-05-31T10:08:12Z

src/main/java/io/nats/client/ConsumerContext.java

+     * @throws IOException covers various communication issues with the NATS
+     *         server such as timeout or interruption
+     * @throws JetStreamApiException the request had an error related to the data
+     */
    ConsumerInfo getConsumerInfo() throws IOException, JetStreamApiException;


Do you think it would be valuable to provide e.g. getCachedConsumerInfo() to be able to get to the consumer info returned when executing consumerContext() or addConsumer() without an extra call to the server?

I added a getCachedConsumerInfo that returns the last one that was read from the server. I added to the doc that technically it could be out of date.

piotrpio · 2023-05-31T10:23:16Z

src/main/java/io/nats/client/ManualConsumer.java

@@ -13,6 +13,30 @@

 package io.nats.client;

+import java.time.Duration;
+
 public interface ManualConsumer extends SimpleConsumer {


Would be nice to have some general docs on the whole interface. I am not a huge fan of the ManualConsumer name to be honest - just an idea of the top of my head, but maybe something like IterableConsumer or even have consumer implement Iterator?

Not strongly against the current naming, I just find it a little bit awkward.

Yeah, I'm did not care for ManualConsumer either. It can't just be Iterator, because since it's endless, I need a way to stop (which is in the superclass) and since this next has a timeout, I can't just make it also implement Java Iterator since it's next does not have a timeout. IterableConsumer is good but not my favorite since Iterator is a well known pattern in java.

I don't even know if Consumer is the best here. For instance the SimpleConsumer is really just a handle/holder/context to allow the dev to stop the consuming since messages are being automatically pushed to the handler they provide. The ManualConsumer is just that plus the next methods.

Naming is hard. Just going to go with IterableConsumer for now

Yeah, naming is hard. In go client, for endless iteration, in addition to having consumer.Consume() - which takes a callback - I added a consumer.Messages() method which works pretty much like ManualConsumer:

it, err := cons.Messages() if err != nil { log.Fatal(err) } for { msg, err := it.Next() if err != nil { fmt.Println("next err: ", err) } msg.Ack() }

piotrpio · 2023-05-31T10:27:13Z

src/main/java/io/nats/client/StreamContext.java

+     *         server such as timeout or interruption
+     * @throws JetStreamApiException the request had an error related to the data
+     */
+    ConsumerContext addConsumer(ConsumerConfiguration config) throws IOException, JetStreamApiException;


Just FYI - not sure which ack policy is default right now, but we recently agreed that at least for the js simplification part (to avoid breaking changes in the old implementation) we should default to AckExplicit). I mention this because go client uses AckNone as default in old implementation and I just changed it for simplified API.

In Java/.NET, when you are creating a ConsumerConfiguration through code, Explicit is the default but of course can be set/changed by the developer. I'm not sure if I understand the comment, this method is accepting the ConsumerConfiguration that the user has supplied, so I can only assume that it's configured the way they want it.

Sure, I just put the comment here because I was curious about the default (having in mind the recent issue in go client) - if it's explicit, that's correct

aricart

LGTM

scottf added 12 commits May 22, 2023 20:56

Simplification Review 1

e3e1fe0

Merge branch 'main' into simplification-review-1

040e80e

Simplification Review 1

9c13e06

documentation

3facb15

test fix and tweaking build to see if it helps tests

cf31616

fixed FetchExample and tuned consume options.

b441fc1

fixed FetchExample, tuned consume options updated pull examples with …

d8680b8

…better exception handling comments

tests and documentation

eefe1f8

tests and documentation

4bb608c

tests and documentation

adbd2cb

simplified error handling in examples

5b96297

filling in stream context api methods

36fcb16

scottf requested review from aricart, levb, Jarema, jnmoyne and piotrpio May 24, 2023 17:08

examples using stream context

e6301f7

MauriceVanVeen reviewed May 24, 2023

View reviewed changes

scottf added 5 commits May 25, 2023 20:18

improve termination

cbeedb9

fixed stop issues

962f82e

handling timeouts

c04bd7d

review while coding .net

0fe508b

review while coding .net

2a075dd

piotrpio reviewed May 31, 2023

View reviewed changes

scottf added 3 commits May 31, 2023 07:52

address review comments

4b71431

address review comments

048cd39

address review comments

ef5a011

aricart approved these changes May 31, 2023

View reviewed changes

scottf merged commit 6f43db6 into main May 31, 2023
2 checks passed

scottf deleted the simplification-review-1 branch May 31, 2023 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplification Review 1 #917

Simplification Review 1 #917

scottf commented May 23, 2023

MauriceVanVeen May 24, 2023

scottf May 24, 2023

MauriceVanVeen May 24, 2023

scottf May 24, 2023

scottf May 24, 2023

MauriceVanVeen May 24, 2023

MauriceVanVeen May 24, 2023

scottf May 24, 2023 •

edited

Loading

MauriceVanVeen May 24, 2023

scottf May 25, 2023

scottf May 25, 2023 •

edited

Loading

MauriceVanVeen May 24, 2023

scottf May 24, 2023

MauriceVanVeen May 24, 2023

piotrpio May 31, 2023

MauriceVanVeen May 24, 2023

scottf May 24, 2023

piotrpio left a comment

piotrpio May 31, 2023

scottf May 31, 2023

piotrpio May 31, 2023

scottf May 31, 2023

piotrpio May 31, 2023

scottf May 31, 2023

piotrpio May 31, 2023

scottf May 31, 2023

piotrpio May 31, 2023

scottf May 31, 2023

piotrpio May 31, 2023

piotrpio May 31, 2023

scottf May 31, 2023

piotrpio May 31, 2023

aricart left a comment

	* @return consumer information.
	* @return a ConsumerContext object

	// Copyright 2021 The NATS Authors
	// Copyright 2023 The NATS Authors

Simplification Review 1 #917

Simplification Review 1 #917

Conversation

scottf commented May 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottf May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottf May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotrpio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aricart left a comment

Choose a reason for hiding this comment

scottf May 24, 2023 •

edited

Loading

scottf May 25, 2023 •

edited

Loading