[changed] JetStream function to not lookup account info & moved it #739

matthiashanel · 2021-06-01T17:50:51Z

The new function is JSAccountInfo

Signed-off-by: Matthias Hanel mh@synadia.com

This separates concerns into give me a jetstream object and check jetstream.
Moving that logic to the nats connection to avoid dependencies with the JetStream call.
In our meeting the group generally favored opt in over opt out.

I'm clearly expecting ppl to either copy a sample anyway and there we add the call of course.
Or if they run into this, to just ask & find this new call.
it's also opt in because the main thing that the lookup does is provide a different error.

I noticed that the unit test for the js object appears to be broken right now. (at least for me, even without my change)
Deleted the checks so I could show you this sooner.

If that approach is not desirec, I'll revert and add an option to opt out of the lookup.

kozlovic

Not commenting on the validity of the move, just noticed inconsistencies.

This is concerning since I just started working on the C client (was waiting for the Go client to be done) and the first function I implemented is the the equivalent of JetStream() (get the context, which would call js.AccountInfo()) and that is already changing :-)

kozlovic · 2021-06-01T21:13:10Z

js.go

@@ -192,15 +192,6 @@ func (nc *Conn) JetStream(opts ...JSOpt) (JetStreamContext, error) {
 	}
 	nc.mu.Unlock()

-	if checkAccount {


If that disappears, then above jsLastCheck should disappear too and be removed from the Conn object.

By the way, existing code is broken: if one calls JetStream() for the first time and say the overall call fails (JetStream not enabled, or custom prefix leads nowhere), then the second call to JetStream() would succeed because we would not check for the AccountInfo().. so this is bad. At the very least if "time check" is maintained, we should record jsLastCheck only on success. But still, it means that the next call to JetStream() may succeed even if the underlying AccountInfo() would have failed because we passed a bad prefix, etc..

fixed. JetStream() is now a constructor only. because it can't reallly fail, we can use js.AccountInfo()

The time check bug was me, and agree we should only stamp (if we keep it) on success.

I am still not clear (and this is me, not the code) as to why we are doing this change vs an option like SkipAccountCheck().

This feels like it could jarring to only catch the you do not have JetStream enabled when you make a secondary call, but maybe that is ok here. Originally before domains and api prefixes it felt like the natural place to check this and return that error.

kozlovic · 2021-06-01T21:15:32Z

nats.go

+		defer cancel()
+	}
+
+	resp, err := nc.RequestWithContext(o.ctx, jOpts.apiSubj(apiAccountInfo), nil)


If user provides a different prefix than the one registered when initially getting the context, then should not this prefix be used instead? In other words, should it be o.apiSubj() here? (if I understand correctly o is the mix of the default's js options and the user provided one.

fixed. instead of moving code we now use js.AcccountInfo (that existed before)

Removded then lookup of account info. now already existing js.AccountInfo has to be called separately. Signed-off-by: Matthias Hanel <mh@synadia.com>

coveralls · 2021-06-02T00:12:20Z

Coverage increased (+0.04%) to 86.978% when pulling efed691 on move-account-lookup into da90d22 on master.

derekcollison · 2021-06-02T01:57:19Z

js.go

@@ -192,15 +192,6 @@ func (nc *Conn) JetStream(opts ...JSOpt) (JetStreamContext, error) {
 	}
 	nc.mu.Unlock()

-	if checkAccount {


The time check bug was me, and agree we should only stamp (if we keep it) on success.

derekcollison · 2021-06-02T01:59:29Z

js.go

@@ -192,15 +192,6 @@ func (nc *Conn) JetStream(opts ...JSOpt) (JetStreamContext, error) {
 	}
 	nc.mu.Unlock()

-	if checkAccount {


I am still not clear (and this is me, not the code) as to why we are doing this change vs an option like SkipAccountCheck().

This feels like it could jarring to only catch the you do not have JetStream enabled when you make a secondary call, but maybe that is ok here. Originally before domains and api prefixes it felt like the natural place to check this and return that error.

derekcollison · 2021-06-02T02:00:06Z

jsm.go

@@ -170,6 +170,8 @@ type accountInfoResponse struct {
 }

 // AccountInfo retrieves info about the JetStream usage from the current account.
+// If JetStream is not enabled, this will return ErrJetStreamNotEnabled


This now has to be the case for every API call to the JS context, but I think that is ok.

@wallyqs we need to probably go through our public calls and make sure that is so

yes could happen if JS becomes disabled

derekcollison · 2021-06-02T02:02:36Z

nats.go

@@ -496,9 +496,6 @@ type Conn struct {
 	respMux   *Subscription        // A single response subscription
 	respMap   map[string]chan *Msg // Request map for the response msg channels
 	respRand  *rand.Rand           // Used for generating suffix
-
-	// JetStream Contexts last account check.
-	jsLastCheck time.Time


I think we all agreed that JS contexts should be lightweight and easy to create. In many tests I create multiple ones with different characteristics.

Question though, do we expect code to look like this all the time or folks will skip the AccountInfo and just check errors on the other API calls?

nc, _ := nc.Connect("demo.nats.io") js, _ := nc.JetStream() if _, err := js.AccountInfo(); err != nil {}

Help me understand: can the enabled/disabled nature of JetStream for this connection change overtime? Meaning, after calling JetStream() (and lets say we still do an account check inside, and it returned ok), is there a possibility that JS may no longer be enabled for this account? What if the connection reconnects to a server where this account does not have JS enabled, which likely would be misconfiguration?

If it can't change, then instead of a time check, it should be simply a "already checked and here is the result" type of things (say 2 booleans).

If it can change overtime and since all APIs will return "not enabled" if JS is or no longer is enabled, not sure what we gain by doing it inside JetStream(). Say you already have the context, but then JS is disabled, then the context is "valid" and yet all other APIs call will fail with "not enabled". Same if you recently got a context and we use time-check, then if we are within the window, JetStream() would return a context (which won't work when calling APIs), but if you happen to ask after the time-check has passed, this call would return "not enabled".

I am not against doing the account check inside JetStream() (with the time-check bug fixed), since it can give a "fail fast" behavior where if you get "not enabled", there is no need to proceed further, but it also can be non deterministic (as described above), and all APIs should be checked for success/failure anyway.

due to reload or jwt update Js can be legitimately enabled disabled on the fly.
We have code that turns a no responder into the same error, so flapping because of that can happen as well.
If you want to fail fast, perhaps for debugging, I'd suggest call js.AccountInfo.
I'd also suggest calling that function on retries. you fails for some reason now you can better determine if retrying makes sense or not.

I think for users that want to ensure that JS is ready and available, a bit more code would be needed which might be better to have outside of the creation of the JetStream() context:

nc, _ := nc.Connect("demo.nats.io") js, _ := nc.JetStream() ctx, done := context.WithTimeout(context.Background(), 5*time.Second) defer done() // IsJSAvailable() for { select { case <-ctx.Done(): return default: } if _, err := js.AccountInfo(); err != nil { // Handle temporary errors: // // - Quorum not ready // - JS API not yet ready // - No responders (temporary while JS booting up) // Then: timeout when too many failures or no response } }

There would be some retries when this happens for example and backoff until is ready.

derekcollison

LGTM

matthiashanel requested review from wallyqs, derekcollison, scottf, variadico and ColinSullivan1 June 1, 2021 17:50

kozlovic reviewed Jun 1, 2021

View reviewed changes

[changed] JetStream function to be a constructor only.

efed691

Removded then lookup of account info. now already existing js.AccountInfo has to be called separately. Signed-off-by: Matthias Hanel <mh@synadia.com>

matthiashanel force-pushed the move-account-lookup branch from f7b41e7 to efed691 Compare June 2, 2021 00:05

derekcollison reviewed Jun 2, 2021

View reviewed changes

derekcollison self-requested a review June 2, 2021 21:25

derekcollison approved these changes Jun 2, 2021

View reviewed changes

matthiashanel merged commit 1267a88 into master Jun 2, 2021

matthiashanel deleted the move-account-lookup branch June 2, 2021 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[changed] JetStream function to not lookup account info & moved it #739

[changed] JetStream function to not lookup account info & moved it #739

matthiashanel commented Jun 1, 2021 •

edited

Loading

kozlovic left a comment

kozlovic Jun 1, 2021

kozlovic Jun 1, 2021

matthiashanel Jun 2, 2021

matthiashanel Jun 2, 2021

derekcollison Jun 2, 2021

derekcollison Jun 2, 2021

kozlovic Jun 1, 2021

matthiashanel Jun 2, 2021

coveralls commented Jun 2, 2021

derekcollison Jun 2, 2021

derekcollison Jun 2, 2021

derekcollison Jun 2, 2021

matthiashanel Jun 2, 2021

wallyqs Jun 2, 2021

derekcollison Jun 2, 2021

kozlovic Jun 2, 2021

matthiashanel Jun 2, 2021

wallyqs Jun 2, 2021

derekcollison left a comment

[changed] JetStream function to not lookup account info & moved it #739

[changed] JetStream function to not lookup account info & moved it #739

Conversation

matthiashanel commented Jun 1, 2021 • edited Loading

kozlovic 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

coveralls commented Jun 2, 2021

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

derekcollison left a comment

Choose a reason for hiding this comment

matthiashanel commented Jun 1, 2021 •

edited

Loading