Skip to content

[pulsar-io-jdbc] no action set as insert #4862

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

Merged
merged 1 commit into from
Aug 2, 2019

Conversation

yittg
Copy link
Contributor

@yittg yittg commented Aug 1, 2019

Motivation

jdbc sink treat all record as INSERT before #4358 , now it requires an indispensable action property which seems to be a break change, and we can deal records without any action property as INSERT.

Modifications

treat action not set as INSERT action like before.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as TestInsertAction.

This change added tests and can be verified as follows: TestNoAction

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

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

Documentation

  • Does this pull request introduce a new feature? (no)

Sorry, something went wrong.

@yittg yittg changed the title [pulsar-io-jdbc] action no set as insert [pulsar-io-jdbc] no action set as insert Aug 1, 2019
@yittg
Copy link
Contributor Author

yittg commented Aug 1, 2019

cc @sijie @tuteng
and seems that Jenkins is broken?

@yittg
Copy link
Contributor Author

yittg commented Aug 1, 2019

Maybe a little more discussion need about whether swallow UNKNOWN action error or export it and trigger fail ack.

@tuteng
Copy link
Member

tuteng commented Aug 1, 2019

Maybe a little more discussion need about whether swallow UNKNOWN action error or export it and trigger fail ack.

I think it is more appropriate to throw an exception

@yittg
Copy link
Contributor Author

yittg commented Aug 1, 2019

@tuteng yeah, agree with this. i prefer to expose it also.

@yittg yittg force-pushed the fix-pulsar-io-jdbc-not-set-action branch from 7ad410b to 3850047 Compare August 1, 2019 10:31
@sijie
Copy link
Member

sijie commented Aug 1, 2019

run cpp tests
run integration tests

@sijie sijie added area/connector type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Aug 1, 2019
@sijie sijie added this to the 2.5.0 milestone Aug 1, 2019
@yittg
Copy link
Contributor Author

yittg commented Aug 2, 2019

@sijie this PR need to be merged manually?

@sijie sijie modified the milestones: 2.5.0, 2.4.1 Aug 2, 2019
@sijie sijie merged commit d8356d8 into apache:master Aug 2, 2019
@sijie
Copy link
Member

sijie commented Aug 2, 2019

@yittg thank you for your contribution!

@yittg yittg deleted the fix-pulsar-io-jdbc-not-set-action branch August 2, 2019 04:11
@yittg
Copy link
Contributor Author

yittg commented Aug 2, 2019

@sijie @tuteng @jiazhai thanks.

jiazhai pushed a commit that referenced this pull request Aug 28, 2019
### Motivation

jdbc sink treat all record as INSERT before #4358 , now it requires an indispensable action property which seems to be a break change, and we can deal records without any action property as INSERT.

### Modifications

treat action not set as INSERT action like before.
(cherry picked from commit d8356d8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connector type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants