Skip to content

Conversation

@NguyenHoangSon96
Copy link
Contributor

@NguyenHoangSon96 NguyenHoangSon96 commented Mar 28, 2025

Issue Link

Proposed Changes

_ Users can set some of the rpc calls options from io.grpcCallOptions (not all, just some).
_ I have to change NettyChannelBuilder.forTarget back to NettyChannelBuilder.forAddress because the latter works well with ip addresses (more flexible).
_ I set maxInboundMessageSize = Integer.MAX_VALUE when the maxInboundMessageSize = null just for backward compatibility reasons.

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • Tests pass
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@NguyenHoangSon96 NguyenHoangSon96 self-assigned this Mar 28, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces support for configuring the maximum RPC inbound message size for the InfluxDB client while also updating the FlightSqlClient to use NettyChannelBuilder.forAddress for improved IP address support.

  • Added unit tests to verify the behavior for different inbound message size settings
  • Updated FlightSqlClient to extract URI and use appropriate NettyChannelBuilder methods
  • Enhanced ClientConfig with maxInboundMessageSize property and updated related methods

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/test/java/com/influxdb/v3/client/config/ClientConfigTest.java Added tests for handling maxInboundMessageSize and minor adjustments in test helpers
src/main/java/com/influxdb/v3/client/internal/FlightSqlClient.java Modified channel creation to use forAddress and added support for configurable message size
src/main/java/com/influxdb/v3/client/config/ClientConfig.java Introduced a new property for maxInboundMessageSize and updated builder, equals, hashCode, and toString

@codecov
Copy link

codecov bot commented Mar 28, 2025

Codecov Report

Attention: Patch coverage is 91.66667% with 8 lines in your changes missing coverage. Please review.

Project coverage is 90.14%. Comparing base (e507aab) to head (282d5c6).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...m/influxdb/v3/client/internal/GrpcCallOptions.java 90.24% 1 Missing and 7 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #232      +/-   ##
==========================================
+ Coverage   90.05%   90.14%   +0.08%     
==========================================
  Files          18       19       +1     
  Lines        1036     1126      +90     
  Branches      164      177      +13     
==========================================
+ Hits          933     1015      +82     
- Misses         40       41       +1     
- Partials       63       70       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@NguyenHoangSon96
Copy link
Contributor Author

Hi @karel-rehor @jansimonb
Codecov complaints about test coverage
But I don't think I can test that code 🧑‍💻
Please review when you have time

@NguyenHoangSon96 NguyenHoangSon96 requested review from jansimonb and karel-rehor and removed request for jansimonb and karel-rehor March 28, 2025 08:14
@NguyenHoangSon96 NguyenHoangSon96 marked this pull request as draft March 28, 2025 08:43
@jansimonb
Copy link
Contributor

jansimonb commented Mar 28, 2025

Does this PR have a linked issue? What is the reason for limiting the message size? Is it ok that the metadata/header size left unlimited?

@NguyenHoangSon96
Copy link
Contributor Author

Does this PR have a linked issue? What is the reason for limiting the message size? Is it ok that the metadata/header size left unlimited?

This is the issue link
https://github.com/orgs/influxdata/projects/108/views/1?pane=issue&itemId=101792742

@jansimonb
Copy link
Contributor

The linked issue also mentions setting the max sent message size. Don't you want to add that too?

@NguyenHoangSon96
Copy link
Contributor Author

The linked issue also mentions setting the max sent message size. Don't you want to add that too?

I talked with Karel and it looks like we need to support all transport options not just the message size limit
I will update my PR accordingly
Thank you for having taken a look 👀

@NguyenHoangSon96 NguyenHoangSon96 changed the title feat: set max rpc message size feat: allows users set some grpc options Apr 2, 2025
@NguyenHoangSon96 NguyenHoangSon96 marked this pull request as ready for review April 2, 2025 03:59
@NguyenHoangSon96 NguyenHoangSon96 requested a review from Copilot April 8, 2025 02:32
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Copy link
Contributor

@jansimonb jansimonb left a comment

Choose a reason for hiding this comment

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

Just minor comments. Please add test for equals/hashcode/tostring to fix the decreased coverage.

Copy link
Contributor

@jansimonb jansimonb left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 👍

@NguyenHoangSon96 NguyenHoangSon96 force-pushed the feat/set-max-message-size branch from a2c80a6 to 282d5c6 Compare April 8, 2025 10:16
@NguyenHoangSon96 NguyenHoangSon96 merged commit 826f5ba into main Apr 10, 2025
9 checks passed
@NguyenHoangSon96 NguyenHoangSon96 deleted the feat/set-max-message-size branch April 10, 2025 08:35
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.

3 participants