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

Add option to use autogenerated id on index requests #510

Open
wants to merge 583 commits into
base: master
Choose a base branch
from

Conversation

tvm18860
Copy link

@tvm18860 tvm18860 commented Apr 7, 2021

Problem

While setting the document ID when indexing does provide exactly once delivery, it does put more load on Elasticsearch and is not necessary for all use cases.

A PR for this issue has been made before (#393). This PR is largely an update to that one, as there were many merge conflicts that needed resolving there as it fell out of date.

Addresses #139 and #97

Solution

Add a new option to use the autogenerated document id on index requests. The new option (use.autogenerated.ids) will default to false and only be applicable when write.method is set to INSERT.

Note that the large diff in the DataCoverter class on the convertRecord method is a result of having to pull a chunk of that code out into a separate method. The checkstyle plugin was throwing errors when I added an extra if statement in that the cyclomatic complexity got too high. The more relevant change is on lines 169-173.

Does this solution apply anywhere else?
  • yes
  • no
If yes, where?

Test Strategy

Testing done:
  • Unit tests
  • Integration tests
  • System tests
  • Manual tests

We are running live connectors with this change now.

Release Plan

@tvm18860 tvm18860 requested a review from a team as a code owner April 7, 2021 23:12
@ghost
Copy link

ghost commented Apr 7, 2021

@confluentinc It looks like @tvm18860 just signed our Contributor License Agreement. 👍

Always at your service,

clabot

@tvm18860 tvm18860 force-pushed the feature/autogen-doc-ids branch 2 times, most recently from 80e22be to cc713d6 Compare June 16, 2021 01:51
@azlev
Copy link

azlev commented Jun 29, 2021

Hello!

I'm interested in this PR. How can I help to merge it?

@tvm18860 tvm18860 force-pushed the feature/autogen-doc-ids branch 2 times, most recently from 6919a6c to 7e8f19b Compare June 29, 2021 22:26
@tvm18860
Copy link
Author

I think this PR is back in a good state now, I had missed a checkstyle validation error after an earlier rebase.

@morrone
Copy link

morrone commented Jan 23, 2023

We would also find this very useful.

Are there any comments from the reviewers about what it would take to get this added? Are there any issues with the approach? Does it just need to be updated again to pass the checks?

@cla-assistant
Copy link

cla-assistant bot commented Sep 11, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

sp-gupta
✅ gjw13
❌ ConfluentJenkins
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet