You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2026/03/08 10:50:26 UTC
[PR] Validate gRPC broker connections on connect [pinot]
xiangfu0 opened a new pull request, #17835:
URL: https://github.com/apache/pinot/pull/17835
## Summary
- validate gRPC broker connections by issuing `select 1;` during `GrpcConnection` initialization
- merge default headers into gRPC request metadata and build JDBC gRPC metadata before session creation
- add a unit test covering the connect-time validation query and metadata merge
## Testing
- `gtimeout 600 ./mvnw -pl pinot-clients/pinot-java-client,pinot-clients/pinot-jdbc-client -am test -Dtest=GrpcConnectionTest -Dsurefire.failIfNoSpecifiedTests=false`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Validate gRPC broker connections on connect [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17835:
URL: https://github.com/apache/pinot/pull/17835#discussion_r2902933999
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/grpc/GrpcConnection.java:
##########
@@ -63,13 +67,73 @@ public GrpcConnection(Properties properties, List<String> brokerList) {
}
public GrpcConnection(Properties properties, BrokerSelector brokerSelector) {
+ this(properties, brokerSelector, ConnectionUtils.getHeadersFromProperties(properties));
+ }
+
+ GrpcConnection(Properties properties, BrokerSelector brokerSelector, Map<String, String> defaultMetadata) {
+ this(properties, brokerSelector, createGrpcQueryClient(properties), defaultMetadata);
+ }
+
+ GrpcConnection(Properties properties, BrokerSelector brokerSelector, BrokerStreamingQueryClient grpcQueryClient) {
+ this(properties, brokerSelector, grpcQueryClient, ConnectionUtils.getHeadersFromProperties(properties));
+ }
+
+ GrpcConnection(Properties properties, BrokerSelector brokerSelector, BrokerStreamingQueryClient grpcQueryClient,
+ Map<String, String> defaultMetadata) {
_brokerSelector = brokerSelector;
- // Convert Properties properties to a Map
- Map<String, Object> propertiesMap = new HashMap<>();
- properties.forEach((key, value) -> propertiesMap.put(key.toString(), value));
- _grpcQueryClient = new BrokerStreamingQueryClient(new GrpcConfig(new PinotConfiguration(propertiesMap)));
+ _grpcQueryClient = grpcQueryClient;
+ _defaultMetadata = Map.copyOf(defaultMetadata);
// Default fail Pinot query if response contains any exception.
_failOnExceptions = Boolean.parseBoolean(properties.getProperty(FAIL_ON_EXCEPTIONS, "TRUE"));
+ validateConnection();
+ }
+
+ private static BrokerStreamingQueryClient createGrpcQueryClient(Properties properties) {
+ Map<String, Object> propertiesMap = new HashMap<>();
+ properties.forEach((key, value) -> propertiesMap.put(key.toString(), value));
+ return new BrokerStreamingQueryClient(new GrpcConfig(new PinotConfiguration(propertiesMap)));
+ }
+
+ private void validateConnection() {
+ try {
+ BrokerResponse brokerResponse =
+ BrokerResponse.fromJson(getJsonResponse(CONNECTION_VALIDATION_QUERY, Collections.emptyMap()));
+ if (brokerResponse.hasExceptions()) {
+ throw new PinotClientException(
+ "Failed to establish gRPC broker connection: " + brokerResponse.getExceptions());
+ }
+ } catch (PinotClientException e) {
+ closeQuietly();
+ throw e;
+ } catch (Exception e) {
+ closeQuietly();
+ throw new PinotClientException("Failed to establish gRPC broker connection", e);
+ }
+ }
Review Comment:
The test only covers the happy path of connection validation. Consider adding a test for the failure path (e.g., when the broker returns an exception or is unreachable) to verify that `closeQuietly()` is invoked and a `PinotClientException` is thrown. For instance, you could have `RecordingBrokerStreamingQueryClient.submit()` throw a `RuntimeException` and assert that the constructor propagates a `PinotClientException`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Validate gRPC broker connections on connect [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #17835:
URL: https://github.com/apache/pinot/pull/17835#discussion_r2903494666
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/grpc/GrpcConnection.java:
##########
@@ -51,10 +53,12 @@
*/
public class GrpcConnection implements AutoCloseable {
public static final String FAIL_ON_EXCEPTIONS = "failOnExceptions";
+ static final String CONNECTION_VALIDATION_QUERY = "select 1;";
Review Comment:
Yes, this is to ensure the broker is accessible and the grpc channel is established.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Validate gRPC broker connections on connect [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #17835:
URL: https://github.com/apache/pinot/pull/17835
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Validate gRPC broker connections on connect [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17835:
URL: https://github.com/apache/pinot/pull/17835#issuecomment-4018895449
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17835?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 55.55%. Comparing base ([`b280619`](https://app.codecov.io/gh/apache/pinot/commit/b2806197e54234a25826eb640ef2233041682508?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`45da434`](https://app.codecov.io/gh/apache/pinot/commit/45da43479405e035b33257b46e3ea4d101193de4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17835 +/- ##
============================================
- Coverage 55.58% 55.55% -0.03%
+ Complexity 727 720 -7
============================================
Files 2497 2494 -3
Lines 141225 141217 -8
Branches 22529 22529
============================================
- Hits 78499 78453 -46
- Misses 56072 56109 +37
- Partials 6654 6655 +1
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17835/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/17835/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/17835/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/17835/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/17835/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17835/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17835/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.55% <ø> (-0.01%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17835/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.55% <ø> (-0.03%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17835/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.55% <ø> (-0.03%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17835/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.55% <ø> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/17835?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Validate gRPC broker connections on connect [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #17835:
URL: https://github.com/apache/pinot/pull/17835#discussion_r2903339227
##########
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/grpc/GrpcConnection.java:
##########
@@ -51,10 +53,12 @@
*/
public class GrpcConnection implements AutoCloseable {
public static final String FAIL_ON_EXCEPTIONS = "failOnExceptions";
+ static final String CONNECTION_VALIDATION_QUERY = "select 1;";
Review Comment:
What is the goal of using this query btw? This will be evaluated purely on the broker without any server dispatch, so it's only to test the client <-> broker path?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org