Skip to content
This repository has been archived by the owner on Dec 16, 2020. It is now read-only.

fix early deletion of wasm state schema #547

Merged
merged 1 commit into from Jun 17, 2020

Conversation

kyessenov
Copy link
Contributor

Signed-off-by: Kuat Yessenov kuat@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Kuat Yessenov <kuat@google.com>
Copy link
Contributor

@mandarjog mandarjog left a comment

Choose a reason for hiding this comment

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

lgtm

How does it manifest in upper layers?
Previously this function did not return an error, what will the user see?

@kyessenov
Copy link
Contributor Author

The return result changed, but it's not checked by metadata exchange plugin at the moment. Behaviourally it's the same as long as double registration happens to be identical.

if (state_prototypes_.find(path) == state_prototypes_.end()) {
state_prototypes_[path] = std::move(state_prototype);
return WasmResult::Ok;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be bad argument only if the new and the old registration is different, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would require equality for the wasm prototypes. In general, it's not clear to me what to do if this method fails in the client. So the client should just log and ignore.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to get rid of protobuf as an argument eventually. The return result is currently ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM.

@mandarjog
Copy link
Contributor

@PiotrSikora @jplevyak We need this in 1.6...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants