Skip to content

GH-37635: [Format][C++][Go] Add app_metadata to FlightInfo and FlightEndpoint #37679

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.

Sign up for GitHub

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

Jump to bottom
Merged
zeroshade merged 13 commits into from
Oct 3, 2023
Merged

GH-37635: [Format][C++][Go] Add app_metadata to FlightInfo and FlightEndpoint #37679

zeroshade merged 13 commits into from
Oct 3, 2023

Conversation

@zeroshade
Copy link
Member

@zeroshade zeroshade commented Sep 12, 2023 •
edited by kou
Loading

Rationale for this change

As suggested here: #37635 (comment) this just adds an app_metadata field to FlightInfo and FlightEndpoint

What changes are included in this PR?

Just the updated proto file and the generated Go code from the proto

Are there any user-facing changes?

Yes.

This PR includes breaking changes to public APIs.

@zeroshade zeroshade requested review from kou and lidavidm September 12, 2023 15:35
@github-actions
Copy link

github-actions bot commented Sep 12, 2023

⚠️ GitHub issue #37635 has been automatically assigned in GitHub to PR creator.

@lidavidm
Copy link
Member

lidavidm commented Sep 12, 2023

Could we also add unit/integration tests?

@zeroshade
Copy link
Member Author

zeroshade commented Sep 12, 2023

@lidavidm currently in the process of doing so

@kou kou changed the title GH-37635: [Format] Add app_metadata to FlightInfo and FlightEndpoint GH-37635: [Format][C++][Go] Add app_metadata to FlightInfo and FlightEndpoint Sep 12, 2023
@kou
Copy link
Member

kou commented Sep 13, 2023

Vote thread: https://lists.apache.org/thread/113xm107ptn1t76txockp11oc7wlh0ok

@lidavidm
Copy link
Member

lidavidm commented Sep 14, 2023

I think we generally require integration tests as well?

@zeroshade
Copy link
Member Author

zeroshade commented Sep 18, 2023

I'll set up integration tests for this sometime this week!

@lidavidm
Copy link
Member

lidavidm commented Sep 25, 2023

Can you rebase? It appears integration tests didn't run

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Sep 25, 2023
@zeroshade
Copy link
Member Author

zeroshade commented Sep 26, 2023

rebased!

kou
kou approved these changes Oct 3, 2023
View reviewed changes
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

@kou
Copy link
Member

kou commented Oct 3, 2023

I'll merge #37867 and rebase this to fix CI failures.

@kou kou force-pushed the flight-metadata branch from abec9c8 to 7683335 Compare October 3, 2023 07:22
@zeroshade
Copy link
Member Author

zeroshade commented Oct 3, 2023

Thanks @kou! Hopefully we can get one more vote on the ML so that we can close the vote and merge this. I'll see if i can poke some people

@pitrou
Copy link
Member

pitrou commented Oct 3, 2023 •
edited
Loading

To be clear, there's no requirement that the app_metadata in a FlightData is the same as the app_metadata for the FlightInfo or FlightEndpoint which led to that data?

@zeroshade
Copy link
Member Author

zeroshade commented Oct 3, 2023

@pitrou correct there's no requirement for it, the integration test is just using that to validate the values are as expected

@zeroshade zeroshade merged commit 92de9a3 into apache:main Oct 3, 2023
@zeroshade zeroshade removed the awaiting merge Awaiting merge label Oct 3, 2023
@zeroshade zeroshade deleted the flight-metadata branch October 3, 2023 17:50
@github-actions github-actions bot added the awaiting review Awaiting review label Oct 3, 2023
@conbench-apache-arrow
Copy link

conbench-apache-arrow bot commented Oct 3, 2023

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 92de9a3.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.

@aiguofer aiguofer mentioned this pull request Oct 4, 2023
Closed
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
@zeroshade @kou @loicalleyne
apacheGH-37635: [Format][C++][Go] Add app_metadata to FlightInfo and …
d595358 
…FlightEndpoint (apache#37679)

### Rationale for this change
As suggested here: apache#37635 (comment) this just adds an `app_metadata` field to FlightInfo and FlightEndpoint

### What changes are included in this PR?
Just the updated proto file and the generated Go code from the proto

### Are there any user-facing changes?

Yes.

**This PR includes breaking changes to public APIs.**

* Closes: apache#37635

Lead-authored-by: Matt Topol <zotthewizard@gmail.com>
Co-authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
@zeroshade @kou @dgreiss
apacheGH-37635: [Format][C++][Go] Add app_metadata to FlightInfo and …
0ffcb35 
…FlightEndpoint (apache#37679)

### Rationale for this change
As suggested here: apache#37635 (comment) this just adds an `app_metadata` field to FlightInfo and FlightEndpoint

### What changes are included in this PR?
Just the updated proto file and the generated Go code from the proto

### Are there any user-facing changes?

Yes.

**This PR includes breaking changes to public APIs.**

* Closes: apache#37635

Lead-authored-by: Matt Topol <zotthewizard@gmail.com>
Co-authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kou kou kou approved these changes

@lidavidm lidavidm lidavidm approved these changes

@assignUser assignUser Awaiting requested review from assignUser assignUser is a code owner

@raulcd raulcd Awaiting requested review from raulcd raulcd is a code owner

Assignees

No one assigned

Labels

awaiting review Awaiting review Component: C++ Component: FlightRPC Component: Go

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FlightRPC] FlightSQL: Expose custom query metadata

4 participants

@zeroshade @lidavidm @kou @pitrou