Skip to content

Conversation

@olalamichelle
Copy link
Contributor

Also added Unit Tests.

@github-actions
Copy link
Contributor

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @bvolpato for label java.
R: @johnjcasey for label io.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@bvolpato
Copy link
Contributor

@Abacn Can you please take a look? You have more context here.

I can see that maxConnections is FieldType.INT16, but in the JdbcIO it's an Integer. Maybe we should make it INT32 instead?

@bvolpato
Copy link
Contributor

By the way, thanks for contributing @olalamichelle! I'm pinging Yi as he has a lot of context around this provider

@bvolpato
Copy link
Contributor

There's a check failing for Spotless, please make sure you run ./gradlew spotlessApply to fix formatting

@olalamichelle
Copy link
Contributor Author

Done.

Copy link
Contributor

@Abacn Abacn left a comment

Choose a reason for hiding this comment

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

Thanks. a previous fix done in #26480 still missing this particular code path (it fixed Python xlang jdbc schemaio but misses java schemaio itself)

@Abacn
Copy link
Contributor

Abacn commented Aug 28, 2023

@Abacn Can you please take a look? You have more context here.

I can see that maxConnections is FieldType.INT16, but in the JdbcIO it's an Integer. Maybe we should make it INT32 instead?

Yeah, the bug indicates the usage of INT16 is error prone -- however, change it to INT32 is equivalently error prone either. Many places need to be in sync

@Abacn Abacn merged commit 33aa34f into apache:master Aug 28, 2023
lostluck pushed a commit to lostluck/beam that referenced this pull request Aug 30, 2023
…eger (apache#28174)

* Add a test'

* Format

---------

Co-authored-by: Fei Xie <feixie@MacBook-Pro.attlocal.net>
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