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 concrete error types to JetStream #874

Merged
merged 5 commits into from
Jun 19, 2023
Merged

Add concrete error types to JetStream #874

merged 5 commits into from
Jun 19, 2023

Conversation

Jarema
Copy link
Member

@Jarema Jarema commented Mar 11, 2023

TODO:

  • add test for nonexisting jetstream error
  • check if JetStreamError is literal tuple in enum where sensible
  • check if what is StreamsError can be JetstreamRequestError (aggregating both request errors and response errors)
  • self-review

@Jarema Jarema force-pushed the jarema/jetstream-errors branch 2 times, most recently from 4b4d17a to 7714869 Compare April 24, 2023 07:22
@Jarema Jarema force-pushed the jarema/jetstream-errors branch 7 times, most recently from 6b48665 to bd17238 Compare June 8, 2023 10:21
@Jarema Jarema changed the title [WIP] Add concrete error types to JetStream Add concrete error types to JetStream Jun 9, 2023
@Jarema Jarema force-pushed the jarema/jetstream-errors branch from 93f0db4 to 96b8f86 Compare June 13, 2023 22:13
@Jarema Jarema marked this pull request as ready for review June 13, 2023 22:13
Copy link
Collaborator

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

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

Good work, some tiny nits / callouts

@@ -0,0 +1,476 @@
// Copyright 2020-2022 The NATS Authors
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is all response related, maybe time for a response module?

use serde::{Deserialize, Serialize};

#[derive(Debug, PartialEq, Eq, Clone, Copy, Deserialize, Serialize)]
pub struct ErrorCode(u64);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this into jetstream::response?

Copy link
Member Author

Choose a reason for hiding this comment

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

as agreed, for now, made jetstream::error module not public ,and exposed Error and ErrorCode


/// `Error` type returned from an API response when an error occurs.
#[derive(Debug, Serialize, Deserialize, Clone, PartialEq)]
pub struct Error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this into jetstream::response?

async-nats/src/jetstream/kv/mod.rs Outdated Show resolved Hide resolved
@@ -132,6 +132,7 @@ use crate::Client;
pub mod account;
pub mod consumer;
pub mod context;
pub mod errors;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think this module can be flattened into response (with into_future requests, we'll probably be having a request module too).

Suggested change
pub mod errors;
pub mod response;

Copy link
Member Author

Choose a reason for hiding this comment

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

Flattened as agreed on chat.
Error accessible via async_nats::jetstream::Error

async-nats/src/jetstream/errors.rs Outdated Show resolved Hide resolved
async-nats/src/jetstream/errors.rs Outdated Show resolved Hide resolved
async-nats/Cargo.toml Outdated Show resolved Hide resolved
@caspervonb caspervonb self-requested a review June 15, 2023 08:41
Copy link
Collaborator

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

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

Two more abbrevs

async-nats/src/jetstream/errors.rs Outdated Show resolved Hide resolved
async-nats/src/jetstream/errors.rs Outdated Show resolved Hide resolved
@Jarema Jarema force-pushed the jarema/jetstream-errors branch 2 times, most recently from ca07d72 to 6f9d4d4 Compare June 16, 2023 09:22
@caspervonb caspervonb self-requested a review June 16, 2023 10:02
Jarema added 5 commits June 19, 2023 13:00
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
Signed-off-by: Tomasz Pietrek <tomasz@nats.io>
@Jarema Jarema force-pushed the jarema/jetstream-errors branch from 6f9d4d4 to cb3edc5 Compare June 19, 2023 11:00
Copy link
Collaborator

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

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

lgtm

@Jarema Jarema merged commit 76789df into main Jun 19, 2023
@Jarema Jarema deleted the jarema/jetstream-errors branch June 19, 2023 12:06
@Jarema Jarema mentioned this pull request Jun 19, 2023
16 tasks
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