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-33925][connectors/opensearch] Allow customising bulk failure handling #39

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

schulzp
Copy link
Contributor

@schulzp schulzp commented Dec 21, 2023

This is a back port of the implementation for the elasticsearch connector, see FLINK-32028, to achieve consistent APIs.

Extracted BulkResponseInspector interface to allow custom handling of (partially) failed bulk requests. If not overridden, default behaviour remains unchanged and partial failures are escalated.

  • fixes FLINK-33925
  • allows custom metrics to be exposed

Copy link

boring-cyborg bot commented Dec 21, 2023

Thanks for opening this pull request! Please check out our contributing guidelines. (https://flink.apache.org/contributing/how-to-contribute.html)

@MartijnVisser MartijnVisser requested a review from reswqa December 27, 2023 15:16
@schulzp
Copy link
Contributor Author

schulzp commented Dec 27, 2023

@reswqa, I checked the CI logs and it looks like TestCodeArchitectureTest breaks the pipeline. It requires that "ITCASE tests should use a MiniCluster resource or extension". I wonder why this becomes an issue as part of this PR.

@hajimeni
Copy link

Hey @schulzp.
Thank you for creating this PR. It would be very helpful if this PR gets merged.
The CI error seems to be caused by differences in files under the archunit-violations path.
It was resolved in the following PR.
#21

Additionally, Flink version has been updated to 1.18.1, Please fix CI target version.

@schulzp
Copy link
Contributor Author

schulzp commented Mar 12, 2024

Hey @hajimeni!

The CI error seems to be caused by differences in files under the archunit-violations path.
It was resolved in the following PR. #21

The PR you mentioned has been merged in 2023, that way before I opened this one. I'd like to help here but I feel lost. If I run mvn clean verify (against flink 1.17.1) all tests pass. Once I add -Dflink.version=1.18.1 the archunit violations show up and are written to the following files:

  • 4382f1f0-807a-45ff-97d8-42f72b6e9484 (updated)
  • eb4a59cf-22af-4223-8dfd-0ebc17ed342d (new file)
  • stored.rules (adds …342d to the list)

What should happen next? Is the build process supposed to just accept those violations instead of failing? Is there any documentation on archunit in the context of flink projects?

Additionally, Flink version has been updated to 1.18.1, Please fix CI target version.

That's out of scope of this PR, isn't it? I looked at the PR workflow for flink-connector-elasticsearch but even there it's 1.18-SNAPSHOT. What is supposed to be changed?

@hajimeni
Copy link

Hey, @schulzp.

I have created a PR for testing.
https://github.com/hajimeni/flink-connector-opensearch/pull/1

CI passes, so it still seems that the differences in the archunit-violations path are relevant.
I am not familiar with archunit or the flink-connector testing mechanism, but I think my PR is the intended situation.

…andling

Extracted `BulkResponseInspector` interface to allow custom handling of (partially) failed bulk requests. If not overridden, default behaviour remains unchanged and partial failures are escalated.

* fixes https://issues.apache.org/jira/browse/FLINK-33925
* allows custom metrics to be exposed
@schulzp schulzp force-pushed the bulk-response-inspector branch from 7114ab3 to 86b6d9b Compare March 13, 2024 07:24
Copy link
Member

@reswqa reswqa left a comment

Choose a reason for hiding this comment

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

Thanks @schulzp, LGTM. And I think I've already reviewed the changes in elasticsearch connector repository.

@reswqa reswqa merged commit 9e161cc into apache:main Mar 14, 2024
5 checks passed
Copy link

boring-cyborg bot commented Mar 14, 2024

Awesome work, congrats on your first merged pull request!

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