You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/23 16:38:36 UTC
[GitHub] [arrow] zeroshade opened a new pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
zeroshade opened a new pull request #10142:
URL: https://github.com/apache/arrow/pull/10142
Adding a convenient way to expose the Application Metadata field in the arrow flight client and server for Go.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a change in pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
Posted by GitBox <gi...@apache.org>.
zeroshade commented on a change in pull request #10142:
URL: https://github.com/apache/arrow/pull/10142#discussion_r619889834
##########
File path: go/arrow/flight/flight_test.go
##########
@@ -311,3 +312,65 @@ func TestServer(t *testing.T) {
t.Fatalf("got %d, want %d", numRows, fi.TotalRecords)
}
}
+
+type flightMetadataWriterServer struct{}
+
+func (f *flightMetadataWriterServer) DoGet(tkt *flight.Ticket, fs flight.FlightService_DoGetServer) error {
+ recs := arrdata.Records[string(tkt.GetTicket())]
+
+ w := flight.NewRecordWriter(fs, ipc.WithSchema(recs[0].Schema()))
+ defer w.Close()
+ for idx, r := range recs {
+ w.WriteWithAppMetadata(r, []byte(fmt.Sprintf("%d_%s", idx, string(tkt.GetTicket()))))
Review comment:
So when running `gofmt` after adding the `/*metadata*/` before `[]byte(fmt.Sprintf("%d_%s", idx, string(tkt.GetTicket())))` it insisted on the comment being put after the `r` before the comma, so it appears that the convention for Go is for the comment identifying the literal to be *after* the value, rather than before it. So that's what i'll do.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #10142:
URL: https://github.com/apache/arrow/pull/10142#issuecomment-826388610
A few minor comments otherwise this looks reasonable. thank you @zeroshade
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a change in pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
Posted by GitBox <gi...@apache.org>.
zeroshade commented on a change in pull request #10142:
URL: https://github.com/apache/arrow/pull/10142#discussion_r619925679
##########
File path: go/arrow/flight/record_batch_reader.go
##########
@@ -60,15 +68,41 @@ func (d *dataMessageReader) Release() {
if d.msg != nil {
d.msg.Release()
d.msg = nil
+ d.lastAppMetadata = nil
}
}
}
+type Reader struct {
+ *ipc.Reader
+ dmr *dataMessageReader
+}
+
+func (r *Reader) Retain() {
Review comment:
added
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
Posted by GitBox <gi...@apache.org>.
zeroshade commented on pull request #10142:
URL: https://github.com/apache/arrow/pull/10142#issuecomment-826908506
@emkornfield I've updated based on your feedback and responded to the questions. Let me know if there's anything else needed
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #10142:
URL: https://github.com/apache/arrow/pull/10142#discussion_r619876590
##########
File path: go/arrow/flight/flight_test.go
##########
@@ -311,3 +312,65 @@ func TestServer(t *testing.T) {
t.Fatalf("got %d, want %d", numRows, fi.TotalRecords)
}
}
+
+type flightMetadataWriterServer struct{}
+
+func (f *flightMetadataWriterServer) DoGet(tkt *flight.Ticket, fs flight.FlightService_DoGetServer) error {
+ recs := arrdata.Records[string(tkt.GetTicket())]
+
+ w := flight.NewRecordWriter(fs, ipc.WithSchema(recs[0].Schema()))
+ defer w.Close()
+ for idx, r := range recs {
+ w.WriteWithAppMetadata(r, []byte(fmt.Sprintf("%d_%s", idx, string(tkt.GetTicket()))))
Review comment:
nit: not sure go convention here. But in other langauges I find it useful to document literal like parameters (the second parameter in this case indicating the second parameter name.
i.e. `/*metadata=*/[]byte(fmt.Sprintf("%d_%s", idx, string(tkt.GetTicket())`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pgwhalen commented on a change in pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
Posted by GitBox <gi...@apache.org>.
pgwhalen commented on a change in pull request #10142:
URL: https://github.com/apache/arrow/pull/10142#discussion_r619418953
##########
File path: go/arrow/flight/record_batch_writer.go
##########
@@ -46,17 +47,39 @@ func (f *flightPayloadWriter) WritePayload(payload ipc.Payload) error {
payload.SerializeBody(&f.buf)
f.fd.DataBody = f.buf.Bytes()
+
return f.w.Send(&f.fd)
}
func (f *flightPayloadWriter) Close() error { return nil }
+type Writer struct {
+ *ipc.Writer
+ pw *flightPayloadWriter
+}
+
+// ClearAppMetadata sets the writers app metadata to nil so that subsequent records
+// will not contain App Metadata until another call is made to WriteWithAppMetadata.
+func (w *Writer) ClearAppMetadata() {
+ w.pw.fd.AppMetadata = nil
+}
Review comment:
Is the idea that you would call `ClearAppMetadata()` after calling `WriteWithAppMetadata()` but before calling `Write()` (with no metadata)? If so, it might be nice to put this pattern in an integration test, since the tests are probably serving as documentation to most users for now (at least it did to me).
That being said, what do you think about just setting `w.pw.fd.AppMetadata = nil` in `WriteWithAppMetadata()` after `w.Write(rec)`? So that you don't have to remember calling `ClearAppMetadata()`.
At least the way I've been using it, "sticky" metadata would be an unfortunate bug, and it would be nice to minimize the chances of that. I suppose my mental model about non-sticky metadata could be wrong, but at least the Java client assumes it is not sticky.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #10142:
URL: https://github.com/apache/arrow/pull/10142#issuecomment-826388610
A few minor comments otherwise this looks reasonable. thank you @zeroshade
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a change in pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
Posted by GitBox <gi...@apache.org>.
zeroshade commented on a change in pull request #10142:
URL: https://github.com/apache/arrow/pull/10142#discussion_r619888556
##########
File path: go/arrow/flight/flight_test.go
##########
@@ -311,3 +312,65 @@ func TestServer(t *testing.T) {
t.Fatalf("got %d, want %d", numRows, fi.TotalRecords)
}
}
+
+type flightMetadataWriterServer struct{}
+
+func (f *flightMetadataWriterServer) DoGet(tkt *flight.Ticket, fs flight.FlightService_DoGetServer) error {
+ recs := arrdata.Records[string(tkt.GetTicket())]
+
+ w := flight.NewRecordWriter(fs, ipc.WithSchema(recs[0].Schema()))
+ defer w.Close()
+ for idx, r := range recs {
+ w.WriteWithAppMetadata(r, []byte(fmt.Sprintf("%d_%s", idx, string(tkt.GetTicket()))))
Review comment:
Go doesn't particularly have a convention for that type of thing, most of the time it's handled by whatever IDE you're using which would be able to tell you what each argument was by hovering over the function. But it's easy to add some comments such as you described, i'll update
##########
File path: go/arrow/flight/flight_test.go
##########
@@ -311,3 +312,65 @@ func TestServer(t *testing.T) {
t.Fatalf("got %d, want %d", numRows, fi.TotalRecords)
}
}
+
+type flightMetadataWriterServer struct{}
+
+func (f *flightMetadataWriterServer) DoGet(tkt *flight.Ticket, fs flight.FlightService_DoGetServer) error {
+ recs := arrdata.Records[string(tkt.GetTicket())]
+
+ w := flight.NewRecordWriter(fs, ipc.WithSchema(recs[0].Schema()))
+ defer w.Close()
+ for idx, r := range recs {
+ w.WriteWithAppMetadata(r, []byte(fmt.Sprintf("%d_%s", idx, string(tkt.GetTicket()))))
Review comment:
So when running `gofmt` after adding the `/*metadata*/` before `[]byte(fmt.Sprintf("%d_%s", idx, string(tkt.GetTicket())))` it insisted on the comment being put after the `r` before the comma, so it appears that the convention for Go is for the comment identifying the literal to be *after* the value, rather than before it. So that's what i'll do.
##########
File path: go/arrow/flight/record_batch_reader.go
##########
@@ -60,15 +68,41 @@ func (d *dataMessageReader) Release() {
if d.msg != nil {
d.msg.Release()
d.msg = nil
+ d.lastAppMetadata = nil
}
}
}
+type Reader struct {
+ *ipc.Reader
+ dmr *dataMessageReader
+}
+
+func (r *Reader) Retain() {
Review comment:
added
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
Posted by GitBox <gi...@apache.org>.
zeroshade commented on pull request #10142:
URL: https://github.com/apache/arrow/pull/10142#issuecomment-825779166
@sbinet @emkornfield
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10142:
URL: https://github.com/apache/arrow/pull/10142#issuecomment-825779175
https://issues.apache.org/jira/browse/ARROW-12517
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #10142:
URL: https://github.com/apache/arrow/pull/10142#discussion_r619876276
##########
File path: go/arrow/flight/record_batch_reader.go
##########
@@ -60,15 +68,41 @@ func (d *dataMessageReader) Release() {
if d.msg != nil {
d.msg.Release()
d.msg = nil
+ d.lastAppMetadata = nil
}
}
}
+type Reader struct {
+ *ipc.Reader
+ dmr *dataMessageReader
+}
+
+func (r *Reader) Retain() {
Review comment:
here and below, these are public methods? It would be nice to add some documentation.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #10142:
URL: https://github.com/apache/arrow/pull/10142#discussion_r619876276
##########
File path: go/arrow/flight/record_batch_reader.go
##########
@@ -60,15 +68,41 @@ func (d *dataMessageReader) Release() {
if d.msg != nil {
d.msg.Release()
d.msg = nil
+ d.lastAppMetadata = nil
}
}
}
+type Reader struct {
+ *ipc.Reader
+ dmr *dataMessageReader
+}
+
+func (r *Reader) Retain() {
Review comment:
here and below, these are public methods? It would be nice to add some documentation.
##########
File path: go/arrow/flight/flight_test.go
##########
@@ -311,3 +312,65 @@ func TestServer(t *testing.T) {
t.Fatalf("got %d, want %d", numRows, fi.TotalRecords)
}
}
+
+type flightMetadataWriterServer struct{}
+
+func (f *flightMetadataWriterServer) DoGet(tkt *flight.Ticket, fs flight.FlightService_DoGetServer) error {
+ recs := arrdata.Records[string(tkt.GetTicket())]
+
+ w := flight.NewRecordWriter(fs, ipc.WithSchema(recs[0].Schema()))
+ defer w.Close()
+ for idx, r := range recs {
+ w.WriteWithAppMetadata(r, []byte(fmt.Sprintf("%d_%s", idx, string(tkt.GetTicket()))))
Review comment:
nit: not sure go convention here. But in other langauges I find it useful to document literal like parameters (the second parameter in this case indicating the second parameter name.
i.e. `/*metadata=*/[]byte(fmt.Sprintf("%d_%s", idx, string(tkt.GetTicket())`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield closed pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
Posted by GitBox <gi...@apache.org>.
emkornfield closed pull request #10142:
URL: https://github.com/apache/arrow/pull/10142
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a change in pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
Posted by GitBox <gi...@apache.org>.
zeroshade commented on a change in pull request #10142:
URL: https://github.com/apache/arrow/pull/10142#discussion_r619888556
##########
File path: go/arrow/flight/flight_test.go
##########
@@ -311,3 +312,65 @@ func TestServer(t *testing.T) {
t.Fatalf("got %d, want %d", numRows, fi.TotalRecords)
}
}
+
+type flightMetadataWriterServer struct{}
+
+func (f *flightMetadataWriterServer) DoGet(tkt *flight.Ticket, fs flight.FlightService_DoGetServer) error {
+ recs := arrdata.Records[string(tkt.GetTicket())]
+
+ w := flight.NewRecordWriter(fs, ipc.WithSchema(recs[0].Schema()))
+ defer w.Close()
+ for idx, r := range recs {
+ w.WriteWithAppMetadata(r, []byte(fmt.Sprintf("%d_%s", idx, string(tkt.GetTicket()))))
Review comment:
Go doesn't particularly have a convention for that type of thing, most of the time it's handled by whatever IDE you're using which would be able to tell you what each argument was by hovering over the function. But it's easy to add some comments such as you described, i'll update
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a change in pull request #10142: ARROW-12517: [Go][Flight] Expose app metadata in flight client and server
Posted by GitBox <gi...@apache.org>.
zeroshade commented on a change in pull request #10142:
URL: https://github.com/apache/arrow/pull/10142#discussion_r619423782
##########
File path: go/arrow/flight/record_batch_writer.go
##########
@@ -46,17 +47,39 @@ func (f *flightPayloadWriter) WritePayload(payload ipc.Payload) error {
payload.SerializeBody(&f.buf)
f.fd.DataBody = f.buf.Bytes()
+
return f.w.Send(&f.fd)
}
func (f *flightPayloadWriter) Close() error { return nil }
+type Writer struct {
+ *ipc.Writer
+ pw *flightPayloadWriter
+}
+
+// ClearAppMetadata sets the writers app metadata to nil so that subsequent records
+// will not contain App Metadata until another call is made to WriteWithAppMetadata.
+func (w *Writer) ClearAppMetadata() {
+ w.pw.fd.AppMetadata = nil
+}
Review comment:
fair point. I've updated this without the ClearAppMetadata and made it no longer "sticky". Metadata only sticks around for a single write.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org