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

changed format of JSClusterNoPeers error #3459

Merged
merged 5 commits into from Sep 9, 2022
Merged

Conversation

matthiashanel
Copy link
Contributor

This error was introduced in #3342 and reveals too much information
This change gets rid of cluster names and peer counts.

All other counts where changed to booleans,
which are only included in the output when the filter was hit.

In addition, the set of not matching tags is included.

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

This error was introduced in #3342 and reveals to much information
This change gets rid of cluster names and peer counts.

All other counts where changed to booleans,
which are only included in the output when the filter was hit.

In addition, the set of not matching tags is included.

Signed-off-by: Matthias Hanel <mh@synadia.com>
Signed-off-by: Matthias Hanel <mh@synadia.com>
@kozlovic
Copy link
Member

kozlovic commented Sep 9, 2022

With a local cluster of 3, when trying to update with a tag (that does not exist), I am getting this with the CLI:

nats: error: could not edit Stream TEST: no suitable peers for placement: peer selection failures: tags not matched {'geo:europess'} (10005)

I think that is good. @derekcollison I think that matches what you wanted, no?

@derekcollison
Copy link
Member

Lots of colons, anyway to cut that down to plain sentence?

no suitable peers for placement: no tags matching 'geo:europess'

`,
e.cluster, e.clusterPeers, e.offline, e.excludeTag, e.noTagMatch, e.noStorage, e.uniqueTag, e.misc)
b := strings.Builder{}
b.WriteString("peer selection")
Copy link
Member

@kozlovic kozlovic Sep 9, 2022

Choose a reason for hiding this comment

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

Wonder if peer selection failures is needed though, because we already return through NewJSClusterNoPeersError() which already has "no suitable peers for placement: "

Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

Still prefer a simpler error that looks more like a sentence. Too many colons.

@kozlovic
Copy link
Member

kozlovic commented Sep 9, 2022

Removing the "peer selection failures: " would result in:

nats: error: could not edit Stream TEST: no suitable peers for placement: tags not matched {'geo:europess'} (10005)

Now, the beginning "nats: error: could not edit Stream TEST:" is from the NATS CLI, so not much we can do there.

@derekcollison
Copy link
Member

Are we sure we are not returning error: prefix from server? Go client puts on nats: prefix.

Would prefer..

nats: could not edit stream 'TEST': no suitable peers for placement, tags not matched {'geo:europess'} (10005)

Note no error:, stream not capitalized, stream name in quotes, and comma vs another colon between placement and tags.

@kozlovic
Copy link
Member

kozlovic commented Sep 9, 2022

@derekcollison That is totally NATS CLI. The response is a stream update response, so that's CLI that format this way. Here is the -trace (if we were to remove the "peer selection failures"

18:32:11 <<< $JS.API.STREAM.UPDATE.TEST
{"type":"io.nats.jetstream.api.v1.stream_update_response","error":{"code":400,"err_code":10005,"description":"no suitable peers for placement: tags not matched {'geo:europess'}"}}

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@derekcollison
Copy link
Member

The tags not matched should be an array, not a map.

Please change to..

no suitable peers for placement: tags not matched ['geo:europess']

samples:
1) no suitable peers for placement, tags not matched ['cloud:GCP', 'country:US']"
2) no suitable peers for placement, insufficient storage

Signed-off-by: Matthias Hanel <mh@synadia.com>
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@derekcollison derekcollison 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: Matthias Hanel <mh@synadia.com>
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

@matthiashanel matthiashanel merged commit f7cb5b1 into main Sep 9, 2022
@matthiashanel matthiashanel deleted the placement-err-format branch September 9, 2022 01:25
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.

None yet

3 participants