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

[Issue 5690][pulsar-io] Make type name of Elasticsearch sink connector configurable. #6028

Merged
merged 5 commits into from Jan 23, 2020

Conversation

@fantapsody
Copy link
Contributor

fantapsody commented Jan 10, 2020

Fixes #5690

Motivation

The default type name cannot be compatible with Elasticsearch versions both before 6.2 and after 7.0, so this change makes it configurable.

Modifications

Make type name configurable, and set the default value to "_doc" to make it compatible with Elasticsearch versions after 6.2 by default.

Verifying this change

Added unit tests and local integration tests.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: yes
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation
@tuteng

This comment has been minimized.

Copy link
Member

tuteng commented Jan 11, 2020

LGTM
Is it convenient for you to write an example document for use, similar to this http://pulsar.apache.org/docs/en/io-cdc-debezium/#example-of-mysql

@tuteng tuteng added the component/io label Jan 11, 2020
@tuteng tuteng added this to the 2.6.0 milestone Jan 11, 2020
@tuteng tuteng requested review from tuteng and jiazhai Jan 11, 2020
@fantapsody

This comment has been minimized.

Copy link
Contributor Author

fantapsody commented Jan 11, 2020

LGTM
Is it convenient for you to write an example document for use, similar to this http://pulsar.apache.org/docs/en/io-cdc-debezium/#example-of-mysql

Sure, I'm working on some end-to-end tests locally, and will deal with it after that.

@fantapsody fantapsody marked this pull request as ready for review Jan 12, 2020
@fantapsody

This comment has been minimized.

Copy link
Contributor Author

fantapsody commented Jan 13, 2020

LGTM
Is it convenient for you to write an example document for use, similar to this http://pulsar.apache.org/docs/en/io-cdc-debezium/#example-of-mysql

@tuteng I have added the example document, could you please review it again?

| `indexNumberOfShards` | int| false |1| The number of shards of the index. |
| `indexNumberOfReplicas` | int| false |1 | The number of replicas of the index. |
| `username` | String| false |" " (empty string)| The username used by the connector to connect to the elastic search cluster. <br><br>If `username` is set, then `password` should also be provided. |
| `password` | String| false | " " (empty string)|The password used by the connector to connect to the elastic search cluster. <br><br>If `username` is set, then `password` should also be provided. |

### Example
## Example

This comment has been minimized.

Copy link
@vzhikserg

vzhikserg Jan 13, 2020

Contributor

IMHO this example section should still belong to the ## Configuration part. ### Configuration looks weird in this context.

This comment has been minimized.

Copy link
@fantapsody

fantapsody Jan 13, 2020

Author Contributor

Thanks, the modification here is to follow the structure of https://raw.githubusercontent.com/apache/pulsar/master/site2/docs/io-cdc-debezium.md . @tuteng @Anonymitaet Could you give some suggestion here?

This comment has been minimized.

Copy link
@vzhikserg

vzhikserg Jan 13, 2020

Contributor

OK, it is fine if you wanted to make it consistent with description of other connectors

@tuteng
tuteng approved these changes Jan 13, 2020
Copy link
Member

tuteng left a comment

LGTM

@tuteng

This comment has been minimized.

Copy link
Member

tuteng commented Jan 13, 2020

retest this please

@sijie
sijie approved these changes Jan 13, 2020
@sijie

This comment has been minimized.

Copy link
Member

sijie commented Jan 13, 2020

retest this please

@sijie

This comment has been minimized.

Copy link
Member

sijie commented Jan 13, 2020

@fantapsody can you take a look at @vzhikserg 's comment?

Co-Authored-By: Sergii Zhevzhyk <vzhikserg@users.noreply.github.com>
@fantapsody

This comment has been minimized.

Copy link
Contributor Author

fantapsody commented Jan 13, 2020

@fantapsody can you take a look at @vzhikserg 's comment?

Done, thanks! @vzhikserg

@sijie

This comment has been minimized.

Copy link
Member

sijie commented Jan 13, 2020

retest this please

1 similar comment
@sijie

This comment has been minimized.

Copy link
Member

sijie commented Jan 14, 2020

retest this please

@sijie sijie closed this Jan 14, 2020
@sijie sijie reopened this Jan 14, 2020
@sijie

This comment has been minimized.

Copy link
Member

sijie commented Jan 17, 2020

run java8 tests

1 similar comment
@sijie

This comment has been minimized.

Copy link
Member

sijie commented Jan 18, 2020

run java8 tests

@sijie

This comment has been minimized.

Copy link
Member

sijie commented Jan 20, 2020

run java8 tests

2 similar comments
@sijie

This comment has been minimized.

Copy link
Member

sijie commented Jan 21, 2020

run java8 tests

@sijie

This comment has been minimized.

Copy link
Member

sijie commented Jan 22, 2020

run java8 tests

@sijie sijie merged commit e23c6a8 into apache:master Jan 23, 2020
34 of 41 checks passed
34 of 41 checks passed
cpp-tests cpp-tests
Details
schema schema
Details
unit-tests unit-tests
Details
unit-tests unit-tests
Details
unit-test-flaky unit-test-flaky
Details
unit-tests unit-tests
Details
cpp-tests
Details
backwards-compatibility
Details
backwards-compatibility
Details
cli
Details
cli
Details
function-state
Details
function-state
Details
messaging
Details
messaging
Details
process
Details
process
Details
schema
Details
sql
Details
sql
Details
standalone
Details
standalone
Details
thread
Details
thread
Details
tiered-filesystem
Details
tiered-filesystem
Details
tiered-jcloud
Details
tiered-jcloud
Details
License check
Details
License check
Details
unit-tests
Details
unit-tests
Details
unit-tests
Details
unit-tests
Details
unit-test-flaky
Details
unit-tests
Details
unit-tests
Details
unit-tests
Details
Jenkins: Java 8 - Unit Tests FAILURE
Details
Jenkins: C++ / Python Tests SUCCESS
Details
Jenkins: Integration Tests SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.