393-js-publish-async and utils / tests / misc #398

scottf · 2021-01-19T18:38:44Z

Misc changes and attempt at #393

scottf · 2021-01-19T18:47:03Z

src/examples/java/io/nats/examples/ExampleUtils.java

+                .name(streamName)
+                .storageType(storageType)
+                .subjects(subject)
+                .build();


convention is to put the dot at the beginning. also when concatenating strings with + it goes at the beginning, or using && for multi line if. It makes it easier to comment out a line too.

scottf · 2021-01-19T18:47:39Z

src/examples/java/io/nats/examples/ExampleUtils.java

+        }
+        return ea;
+    }
+


I noticed that this was repeated so I refactored it.

scottf · 2021-01-19T18:49:10Z

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

+    CompletableFuture<PublishAck> publishAsync(String subject, byte[] body);
+    CompletableFuture<PublishAck> publishAsync(String subject, byte[] body, PublishOptions options);
+    CompletableFuture<PublishAck> publishAsync(Message message);
+    CompletableFuture<PublishAck> publishAsync(Message message, PublishOptions options);


This for #393

Looks good, just needs comments.

scottf · 2021-01-19T18:51:29Z

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

@@ -438,7 +432,7 @@ static DecodedSeed decodeSeed(char[] seed) {
    private static NKey createPair(Type type, SecureRandom random)
            throws IOException, NoSuchProviderException, NoSuchAlgorithmException {
        if (random == null) {
-            random = new SecureRandom();
+            random = SRAND;


shared secure random

scottf · 2021-01-19T18:51:55Z

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

-    private static final String QCOLONQ = "\": \"";
-    private static final String QCOLON = "\": ";
+    private static final String QCOLONQ = "\":\"";
+    private static final String QCOLON = "\":";


Space is not required. Not need to send extra bytes

scottf · 2021-01-19T18:52:46Z

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

@@ -65,22 +66,22 @@
        AccountLimitImpl(String json) {
            Matcher m = LIMITS_MEMORY_RE.matcher(json);
            if (m.find()) {
-                this.memory = Integer.parseInt(m.group(1));
+                this.memory = Long.parseLong(m.group(1));


Use the long parser since the variables are long.

scottf · 2021-01-19T18:52:57Z

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

+            }
+        });
+    }
+


This for #393

scottf · 2021-01-19T18:53:59Z

src/main/java/io/nats/client/support/RandomUtils.java

+import java.security.SecureRandom;
+import java.util.Random;
+
+public abstract class RandomUtils {


Utils provides shared random. Both Random and Secure random are thread-safe.

scottf · 2021-01-19T18:54:44Z

src/test/java/io/nats/client/JetstreamTests.java

-    }
-
-    private static StreamInfo createMemoryStream(JetStream js, String streamName, String[] subjects)
+    private static StreamInfo createMemoryStream(JetStream js, String streamName, String... subjects)


This signature will accept one (zero actually) or more strings, so we don't need 2 methods

scottf · 2021-01-19T18:57:18Z

src/test/java/io/nats/client/JetstreamTests.java

-                Assertions.fail("Exception:  " + ex.getMessage());
-            } finally {
-                nc.close();
-            }


If an exception is throw the test will fail naturally, so no need to check for it.

Finally to close the connection is not required since Connection implements AutoCloseable and the object is created via the try, the close happens automatically.

scottf · 2021-01-19T18:58:00Z

src/test/java/io/nats/client/JetstreamTests.java

+                // check success
+                 opts.setExpectedStream("foo-stream");
+                 opts.setExpectedLastSeqence(PublishOptions.unsetLastSequence);
+                 js.publish("foo", null, opts);


this now passes since the server is fixed.

ColinSullivan1 · 2021-01-21T00:05:02Z

src/examples/java/io/nats/examples/NatsJsManage.java

+            catch (IllegalStateException ise) {
+                System.out.println(ise.getMessage());
+            }
+/*


Did you intend to leave this in?

ColinSullivan1 · 2021-01-21T00:05:40Z

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

+
+package io.nats.client;
+
+// TODO


Need class comment...

ColinSullivan1 · 2021-01-26T18:04:13Z

MAINTAINERS.md

@@ -8,3 +8,4 @@ Maintainership is on a per project basis.
  - Colin Sullivan <colin@nats.io> [@ColinSullivan1](https://github.com/ColinSullivan1)
  - Derek Collison <derek@nats.io> [@derekcollison](https://github.com/derekcollison)
  - Ivan Kozlovic <ivan@nats.io> [@kozlovic](https://github.com/kozlovic)
+  - Scott Fauerbach <scott@nats.io> [@scottf](https://github.com/scottf)


Unfortunately we can't do this right now. We need a formal vote to add you as a Maintainer per NATS Governance. We'll certainly revisit this!

ok sorry, will remove

ColinSullivan1 · 2021-01-26T19:46:24Z

src/examples/java/io/nats/examples/NatsJsManage.java

+            action("Make And Use Subscription");
+            SubscribeOptions so = SubscribeOptions.builder()
+                    .pullDirect(STREAM1, "GQJ3IvWo", 10)
+//                    .configuration(STREAM1, cc)


Intended to be removed?

This is an example that I have not finished

ColinSullivan1 · 2021-01-26T19:50:33Z

src/examples/java/io/nats/examples/NatsJsManage.java

+    }
+
+    private static void action(String label) {
+        System.out.println("================================================================================");


Looks like an issue with spacing?

Just debug stuff using to show what's going on

ColinSullivan1 · 2021-01-26T20:00:43Z

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

+     */
+    PublishAck publish(Message message, PublishOptions options) throws IOException, JetStreamApiException;
+
+    CompletableFuture<PublishAck> publishAsync(String subject, byte[] body);


Needs comments...

ColinSullivan1 · 2021-01-26T20:02:52Z

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

+
+package io.nats.client.impl;
+
+public class JetStreamApiException extends Exception {


This class and methods will need doc... may not make it through travis.

Will comment for next PR

ColinSullivan1 · 2021-01-26T20:10:29Z

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

+
+    private Message makeRequest(NatsMessage natsMessage, Duration timeout) throws IOException {
+        try {
+            return responseRequired(conn.request(natsMessage, timeout));


I think this'll miss the prefix...

ColinSullivan1

Lots of work here! LGTM so far - a few comments on docs, etc.

ColinSullivan1

Per offline discussion, after comments are added LGTM. Feel free to merge..

scottf added 3 commits December 30, 2020 20:27

393-js-publish-async

c551fc2

Merge remote-tracking branch 'origin/master'

39e40de

RandomUtils-tests-misc

b6db80a

scottf commented Jan 19, 2021

View reviewed changes

scottf changed the title ~~Utils tests misc~~ 393-js-publish-async and utils / tests / misc Jan 19, 2021

scottf commented Jan 19, 2021

View reviewed changes

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

}

});

}

Copy link

Contributor Author

scottf Jan 19, 2021

Choose a reason for hiding this comment

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

This for #393

scottf commented Jan 19, 2021

View reviewed changes

management

9046d1d

ColinSullivan1 reviewed Jan 21, 2021

View reviewed changes

src/main/java/io/nats/client/ConsumerLister.java Outdated

package io.nats.client;

// TODO

Copy link

Member

ColinSullivan1 Jan 21, 2021 •

edited

Choose a reason for hiding this comment

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

Need class comment...

scottf added 2 commits January 25, 2021 14:26

management more (1)

0b156a5

documentation and cleanup

fa49d76

scottf mentioned this pull request Jan 26, 2021

Jetstream durable name validation issue nats-io/nats.js#382

Closed

scottf added 2 commits January 26, 2021 10:27

documentation and cleanup (2)

25b0b14

consumer/durable name validation

cb2c401

ColinSullivan1 reviewed Jan 26, 2021

View reviewed changes

consumer info and stream info tests

8e4fcf5

ColinSullivan1 reviewed Jan 26, 2021

View reviewed changes

ColinSullivan1 requested changes Jan 26, 2021

View reviewed changes

ColinSullivan1 approved these changes Jan 26, 2021

View reviewed changes

added javadoc

4060727

scottf merged commit ce1c050 into nats-io:master Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

393-js-publish-async and utils / tests / misc #398

393-js-publish-async and utils / tests / misc #398

scottf commented Jan 19, 2021 •

edited

scottf Jan 19, 2021

scottf Jan 19, 2021

scottf Jan 19, 2021 •

edited

ColinSullivan1 Jan 21, 2021

scottf Jan 19, 2021

scottf Jan 19, 2021 •

edited

scottf Jan 19, 2021

scottf Jan 19, 2021

scottf Jan 19, 2021

scottf Jan 19, 2021

scottf Jan 19, 2021

scottf Jan 19, 2021

ColinSullivan1 Jan 21, 2021

ColinSullivan1 Jan 21, 2021 •

edited

ColinSullivan1 Jan 26, 2021

scottf Jan 26, 2021

ColinSullivan1 Jan 26, 2021

scottf Jan 26, 2021

ColinSullivan1 Jan 26, 2021

scottf Jan 26, 2021

ColinSullivan1 Jan 26, 2021

ColinSullivan1 Jan 26, 2021

scottf Jan 26, 2021

ColinSullivan1 Jan 26, 2021

ColinSullivan1 left a comment

ColinSullivan1 left a comment


		package io.nats.client.impl;

		public class JetStreamApiException extends Exception {

393-js-publish-async and utils / tests / misc #398

393-js-publish-async and utils / tests / misc #398

Conversation

scottf commented Jan 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottf Jan 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottf Jan 19, 2021 • edited

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

ColinSullivan1 Jan 21, 2021 • edited

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

ColinSullivan1 left a comment

Choose a reason for hiding this comment

ColinSullivan1 left a comment

Choose a reason for hiding this comment

scottf commented Jan 19, 2021 •

edited

scottf Jan 19, 2021 •

edited

scottf Jan 19, 2021 •

edited

ColinSullivan1 Jan 21, 2021 •

edited