Skip to content

Log exception in ProcessImpl::run()#327

Merged
mum4k merged 7 commits intoenvoyproxy:masterfrom
oschaaf:run-exception-logging
Apr 22, 2020
Merged

Log exception in ProcessImpl::run()#327
mum4k merged 7 commits intoenvoyproxy:masterfrom
oschaaf:run-exception-logging

Conversation

@oschaaf
Copy link
Copy Markdown
Member

@oschaaf oschaaf commented Apr 20, 2020

Related to #317

Log any exceptions and rethrow. This is to assist with
diagnosis once it happens again in the CI env.

Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com

oschaaf added 2 commits April 20, 2020 16:53
Related to envoyproxy#317

Log any exceptions and rethrow. This is to assist with
diagnosis once it happens again in the CI env.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf oschaaf added P1 waiting-for-review A PR waiting for a review. labels Apr 20, 2020
cluster_manager_ = cluster_manager_factory_->clusterManagerFromProto(bootstrap);
maybeCreateTracingDriver(bootstrap.tracing());
cluster_manager_->setInitializedCb([this]() -> void { init_manager_.initialize(init_watcher_); });
try {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This was already a fairly long function to begin with. How hard would it be to refactor the content of the new try { } into a separate private method? That way we could separate the exception handling (the outer run method) from the business logic (the new private method) and make this a bit easier to follow.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in 9c89e74

@oschaaf
Copy link
Copy Markdown
Member Author

oschaaf commented Apr 21, 2020

@mum4k
Copy link
Copy Markdown
Collaborator

mum4k commented Apr 21, 2020

Is the flaky coverage something we should address / track?

Also can you help me by triggering it again so this becomes able to merge?

@mum4k mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Apr 21, 2020
@oschaaf
Copy link
Copy Markdown
Member Author

oschaaf commented Apr 21, 2020

Is the flaky coverage something we should address / track?

Also can you help me by triggering it again so this becomes able to merge?

I'm trying to address this in-place, it should be possible to reliably hit the line I mentioned.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf
Copy link
Copy Markdown
Member Author

oschaaf commented Apr 21, 2020

I punted the coverage flake to #328
Also, added e28e185 which has cleanup but should be a no-op. At this time I can't understand how the line I pointed out above could not be covered.

@oschaaf oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Apr 21, 2020
mum4k
mum4k previously approved these changes Apr 21, 2020
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@mum4k mum4k merged commit c9ffaf6 into envoyproxy:master Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 waiting-for-review A PR waiting for a review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants