-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-38022: [Java][FlightRPC] Expose app_metadata on FlightInfo and FlightEndpoint #38331
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
Conversation
|
|
| * @param appMetadata (optional) Application metadata associated with this endpoint. | ||
| * @param locations The possible locations the stream can be retrieved from. | ||
| */ | ||
| public FlightEndpoint(Ticket ticket, Instant expirationTime, String appMetadata, Location... locations) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I add a constructor for FlightEndpoint(Ticket ticket, String appMetadata, Location... locations)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can omit that. A builder could be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to private and added builder.
java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightEndpoint.java
Outdated
Show resolved
Hide resolved
java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightInfo.java
Show resolved
Hide resolved
java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightEndpoint.java
Outdated
Show resolved
Hide resolved
java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightEndpoint.java
Outdated
Show resolved
Hide resolved
| * @param appMetadata (optional) Application metadata associated with this endpoint. | ||
| * @param locations The possible locations the stream can be retrieved from. | ||
| */ | ||
| public FlightEndpoint(Ticket ticket, Instant expirationTime, String appMetadata, Location... locations) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can omit that. A builder could be added.
java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightInfo.java
Outdated
Show resolved
Hide resolved
java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightEndpoint.java
Show resolved
Hide resolved
java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightInfo.java
Show resolved
Hide resolved
lidavidm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightEndpoint.java
Show resolved
Hide resolved
java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightInfo.java
Show resolved
Hide resolved
|
You'll want to rebase again, also unskip Java here: arrow/dev/archery/archery/integration/runner.py Lines 615 to 619 in 6f8f34b
|
|
Ah, sorry - then you need to implement the integration test scenario for Java to do the same thing as Go/C++: add an AppMetadataFlightInfoEndpointScenario.java at https://github.com/apache/arrow/tree/main/java/flight/flight-integration-tests/src/main/java/org/apache/arrow/flight/integration/tests following appMetadataFlightInfoEndpointScenarioTester in Go (
|
|
No worries! Started on this but the java setup is quite different. For the Producer, should I extend |
|
Start with |
|
Something interesting I noticed from the Go test... the |
It's Protobuf, so any field can be omitted, but this is the same as having a ticket whose value is an empty string. I don't think we need to expose this separately in other languages. |
lidavidm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
|
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 57f643c. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…nd FlightEndpoint (apache#38331) Making necessary changes in Java to expose the newly added app_metadata. * Closes: apache#38022 Authored-by: Diego Fernandez <aiguo.fernandez@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
…nd FlightEndpoint (apache#38331) Making necessary changes in Java to expose the newly added app_metadata. * Closes: apache#38022 Authored-by: Diego Fernandez <aiguo.fernandez@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
…nd FlightEndpoint (apache#38331) Making necessary changes in Java to expose the newly added app_metadata. * Closes: apache#38022 Authored-by: Diego Fernandez <aiguo.fernandez@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>
Making necessary changes in Java to expose the newly added app_metadata.