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

runtime-v2: flush events on process error #1001

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Conversation

brig
Copy link
Contributor

@brig brig commented Oct 22, 2024

and EventReportingService does not implement the ExecutionListener interface, as it's not supposed to :)

@brig brig requested review from benbroadaway and a team October 22, 2024 15:52
@brig
Copy link
Contributor Author

brig commented Oct 22, 2024

We probably don't need the EventReportingService interface... What do you think?

@ibodrov
Copy link
Collaborator

ibodrov commented Oct 22, 2024

We probably don't need the EventReportingService interface... What do you think?

Agree, let's remote it. I guess the original idea was to provide alternative implementations in, for example, the CLI -- i.e. print out events instead of sending to the server. But I see that in the CLI we just provide our own Set.

@brig
Copy link
Contributor Author

brig commented Oct 22, 2024

We probably don't need the EventReportingService interface... What do you think?

Agree, let's remote it. I guess the original idea was to provide alternative implementations in, for example, the CLI -- i.e. print out events instead of sending to the server. But I see that in the CLI we just provide our own Set.

will remove it in separate PR. after this one: #1002

@brig brig merged commit d168f95 into master Oct 22, 2024
4 checks passed
@brig brig deleted the brig/v2-events-flush-fix branch October 22, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants