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

[FLINK-31993][runtime] Initialize and pass down FailureEnrichers #22516

Merged
merged 1 commit into from
May 31, 2023

Conversation

pgaref
Copy link
Contributor

@pgaref pgaref commented May 4, 2023

https://issues.apache.org/jira/browse/FLINK-31993

  • Init FailureEnrichers as part of ClusterEntrypoint (to avoid performing the same task per JM initialization)

The set of Enrichers is then passed down to JM and Scheduler implementations as needed.

@flinkbot
Copy link
Collaborator

flinkbot commented May 4, 2023

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@pgaref pgaref changed the title [FLINK-31993][runtime] Initialize and pass down JobMaster FailureEnrichers [FLINK-31993][runtime] Initialize and pass down FailureEnrichers May 23, 2023
@pgaref pgaref force-pushed the FLINK-31993 branch 2 times, most recently from 74fbcfa to bdae163 Compare May 23, 2023 15:52
Copy link
Member

@dmvk dmvk left a comment

Choose a reason for hiding this comment

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

LGTM overall, minus a few cosmetic comments. I think the commit message is also misleading

Fail fast on bad FailureEnricher JM startup as given by the user

since we still have filterInvalidEnrichers in the codepath

* Init FailureEnrichers as part of ClusterEntrypoint
@pgaref
Copy link
Contributor Author

pgaref commented May 26, 2023

LGTM overall, minus a few cosmetic comments. I think the commit message is also misleading

Fail fast on bad FailureEnricher JM startup as given by the user

since we still have filterInvalidEnrichers in the codepath

Awesome! Thanks @dmvk ! Updated

Copy link
Member

@dmvk dmvk left a comment

Choose a reason for hiding this comment

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

👍 LGTM, good job; we should probably follow-up on the MiniCluster support

@@ -561,6 +561,7 @@ private void setupDispatcherResourceManagerComponents(
metricRegistry,
new MemoryExecutionGraphInfoStore(),
metricQueryServiceRetriever,
Collections.emptySet(),
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for not supporting the minicluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be done as part of FLINK-31895

@dmvk dmvk merged commit 603181d into apache:master May 31, 2023
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.

3 participants