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

feat: [MR-552] Define RejectCode::SysUnknown and error codes #1380

Merged
merged 6 commits into from
Sep 11, 2024

Conversation

alin-at-dfinity
Copy link
Contributor

@alin-at-dfinity alin-at-dfinity commented Sep 8, 2024

[MR-552] Define new RejectCode variant SysUnknown (for when the response to a best-effort call is unknown) and two ErrorCode variants -- DeadlineExpired and ResponseDropped -- to be used when expiring best-effort calls or, respectively, when dropping best-effort responses.

Define new `RejectCode` variant `SysUnknown` (for when the response to a best-effort call is unknown) and two `ErrorCode` variants -- `DeadlineExpired` and `ResponseDropped` -- to be used when expiring best-effort calls or dropping best-effort responses.
@alin-at-dfinity
Copy link
Contributor Author

Technically, we should also have a new certification version to go along with it (e.g. V19 does not support SysUnknown, V20 does), but the value is encoded as a plain u8, so it would basically only be "forward compatibility theater": i.e. it would only serve as a marker for "support for this needs to roll out to all subnets before it can be enabled on any subnet" (or the other way around).

I can do it, but it's definitely not necessary in any way. WDYT?

@alin-at-dfinity alin-at-dfinity requested a review from a team as a code owner September 8, 2024 12:08
@rumenov
Copy link
Member

rumenov commented Sep 9, 2024

@alin-at-dfinity one car argue that DeadlineExpired and ResponseDropped are SysTransient, right?

@rumenov rumenov self-requested a review September 9, 2024 06:44
@stiegerc
Copy link
Contributor

stiegerc commented Sep 9, 2024

RejectCode is included in the reject response. The canister can access it using this. I wonder whether this provides any useful information from the point of view of a canister, i.e. what does SysUnknown tell me that SysTransient doesn't in terms of how I should react?

I see you also defined new ErrorCode variants, which are use for the execution history. But we could also map those to SysTransient right?

@derlerd-dfinity
Copy link
Contributor

derlerd-dfinity commented Sep 9, 2024

@rumenov, @stiegerc we decided against using SysTransient because Unknown means it might not be safe to blindly retry (as the message might have reached the target canister and was successfully executed) whereas Transient means it is safe to blindly retry [it is a separate, unrelated discussion whether or not blindly retrying is a good idea from an application perspective].

Copy link
Contributor

@derlerd-dfinity derlerd-dfinity left a comment

Choose a reason for hiding this comment

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

Code wise the change LGTM.

I'd suggest to wait a bit to see whether there are strong objections against this new reject code based on the questions/discussions above.

rs/types/types/src/exhaustive.rs Show resolved Hide resolved
Copy link
Member

@oggy-dfin oggy-dfin left a comment

Choose a reason for hiding this comment

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

LGTM, is in line with the proposed interface spec change

@oggy-dfin
Copy link
Member

One thing I don't know is if we're still maintaining the list of error codes - it's definitely gotten stale, but if we want to maintain it, we should add these codes there at some point.

@stiegerc
Copy link
Contributor

stiegerc commented Sep 9, 2024

@rumenov, @stiegerc we decided against using SysTransient because Unknown means it might not be safe to blindly retry (as the message might have reached the target canister and was successfully executed) whereas Transient means it is safe to blindly retry [it is a separate, unrelated discussion whether or not blindly retrying is a good idea from an application perspective].

Ah I see, makes sense.

rs/types/types/src/exhaustive.rs Show resolved Hide resolved
@rumenov
Copy link
Member

rumenov commented Sep 10, 2024

as the message might have reached the target canister and was successfully executed

Why if the message was successfully executed it cannot be retried ?

@stiegerc
Copy link
Contributor

as the message might have reached the target canister and was successfully executed

Why if the message was successfully executed it cannot be retried ?

It can but it may still be important information for the canister that the message may have arrived, whereas with SysTransient you know for sure that it didn't. It stands to reason that you should use a guaranteed reply message for this kind of thing but that's a different discussion.

@alin-at-dfinity alin-at-dfinity added this pull request to the merge queue Sep 11, 2024
Merged via the queue into master with commit bfba799 Sep 11, 2024
25 checks passed
@alin-at-dfinity alin-at-dfinity deleted the alin/MR-552-SysUnknown branch September 11, 2024 10:08
levifeldman pushed a commit to levifeldman/ic that referenced this pull request Oct 1, 2024
…ty#1380)

[[MR-552]] Define new `RejectCode` variant `SysUnknown` (for when the
response to a best-effort call is unknown) and two `ErrorCode` variants
-- `DeadlineExpired` and `ResponseDropped` -- to be used when expiring
best-effort calls or, respectively, when dropping best-effort responses.

[MR-552]:
https://dfinity.atlassian.net/browse/MR-552?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants