Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add new ModuleRuntime::list_with_details #503

Merged
merged 6 commits into from
Nov 1, 2018

Conversation

arsing
Copy link
Member

@arsing arsing commented Oct 30, 2018

Some of the existing code used ModuleRuntime::list to list all modules,
then called Module::runtime_state for each of them. This can fail if a
module is deleted after it's returned in the list result and before
its runtime_state is called.

The new list_with_details API filters out those modules whose
runtime_state fails with NotFound.

Some of the existing code used `ModuleRuntime::list` to list all modules,
then called `Module::runtime_state` for each of them. This can fail if a
module is deleted after it's returned in the `list` result and before
its `runtime_state` is called.

The new `list_with_details` API filters out those modules whose
`runtime_state` fails with `NotFound`.
@arsing arsing requested review from myagley, damonbarry and avranju and removed request for myagley and damonbarry October 30, 2018 22:50
avranju
avranju previously approved these changes Nov 1, 2018
Copy link
Contributor

@avranju avranju left a comment

Choose a reason for hiding this comment

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

LGTM

.map(|module| module.runtime_state().map(|state| (module, state))),
)
}).flatten()
.then(Ok::<_, Error>) // Ok(_) -> Ok(Ok(_)), Err(_) -> Ok(Err(_)), ! -> Err(_)
Copy link
Contributor

Choose a reason for hiding this comment

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

Took a while for me to understand what's going on there! :)

Copy link
Contributor

@avranju avranju left a comment

Choose a reason for hiding this comment

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

Looks good. Curious though as to why you decided to change the implementation from using a Stream impl to a combinator based one.

@arsing arsing merged commit 645545a into Azure:master Nov 1, 2018
@arsing
Copy link
Member Author

arsing commented Nov 1, 2018

I'd written the custom impl because I'd forgotten about the "lift with Ok" trick for working with errors. (Haven't used futures 0.1 in a long time.) Remembered the trick overnight.

@avranju
Copy link
Contributor

avranju commented Nov 1, 2018

Ah. Got it.

@arsing arsing deleted the list-with-details branch November 1, 2018 20:42
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants