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 2022/03/14 20:34:27 UTC
[GitHub] [arrow] lidavidm opened a new pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
lidavidm opened a new pull request #12628:
URL: https://github.com/apache/arrow/pull/12628
Expand the common Flight suite with some edge cases discovered while testing UCX (ARROW-15706). Also, factor the Flight client's handling of non-host memory types out of the gRPC transport and into the common code.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12628:
URL: https://github.com/apache/arrow/pull/12628#issuecomment-1071243526
Benchmark runs are scheduled for baseline = 331fc4476b6c38c85d4403f1b9726111b4c28f26 and contender = 6a2c432ea715bd8f4975c4c3b303135030bfe3c2. 6a2c432ea715bd8f4975c4c3b303135030bfe3c2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/55b94f53ea87449083d6e7b67c8f3ca0...35c66407d39143d288b257bb2a5eb6a2/)
[Finished :arrow_down:0.13% :arrow_up:0.04%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/ecc3532adad14c65a763214a52a2ab5a...43e573230eb840c0849c5d81db34a861/)
[Finished :arrow_down:0.36% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/b2eeb4acd21046d49691b95213d855c7...0f8bc160ff2840a890ec6b86ed74e1d5/)
[Finished :arrow_down:0.81% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/48f389de751c4ffaa0f76dba16ccf1db...3f3551cc86e84164afdbb942c8a8995f/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm closed pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
lidavidm closed pull request #12628:
URL: https://github.com/apache/arrow/pull/12628
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #12628:
URL: https://github.com/apache/arrow/pull/12628#discussion_r826970540
##########
File path: cpp/src/arrow/flight/flight_test.cc
##########
@@ -81,6 +81,7 @@ TEST_F(GrpcConnectivityTest, GetPort) { TestGetPort(); }
TEST_F(GrpcConnectivityTest, BuilderHook) { TestBuilderHook(); }
TEST_F(GrpcConnectivityTest, Shutdown) { TestShutdown(); }
TEST_F(GrpcConnectivityTest, ShutdownWithDeadline) { TestShutdownWithDeadline(); }
+TEST_F(GrpcConnectivityTest, BrokenConnection) { TestBrokenConnection(); }
Review comment:
I added convenience macros. GTest's value-parameterized tests would work better for this but I couldn't get the Windows build to work out (GTest thought that the same test fixture had been defined multiple times and would error on startup)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12628:
URL: https://github.com/apache/arrow/pull/12628#issuecomment-1071243526
Benchmark runs are scheduled for baseline = 331fc4476b6c38c85d4403f1b9726111b4c28f26 and contender = 6a2c432ea715bd8f4975c4c3b303135030bfe3c2. 6a2c432ea715bd8f4975c4c3b303135030bfe3c2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/55b94f53ea87449083d6e7b67c8f3ca0...35c66407d39143d288b257bb2a5eb6a2/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/ecc3532adad14c65a763214a52a2ab5a...43e573230eb840c0849c5d81db34a861/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/b2eeb4acd21046d49691b95213d855c7...0f8bc160ff2840a890ec6b86ed74e1d5/)
[Finished :arrow_down:0.81% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/48f389de751c4ffaa0f76dba16ccf1db...3f3551cc86e84164afdbb942c8a8995f/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12628:
URL: https://github.com/apache/arrow/pull/12628#issuecomment-1067265670
https://issues.apache.org/jira/browse/ARROW-15932
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #12628:
URL: https://github.com/apache/arrow/pull/12628#issuecomment-1071243526
Benchmark runs are scheduled for baseline = 331fc4476b6c38c85d4403f1b9726111b4c28f26 and contender = 6a2c432ea715bd8f4975c4c3b303135030bfe3c2. 6a2c432ea715bd8f4975c4c3b303135030bfe3c2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/55b94f53ea87449083d6e7b67c8f3ca0...35c66407d39143d288b257bb2a5eb6a2/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/ecc3532adad14c65a763214a52a2ab5a...43e573230eb840c0849c5d81db34a861/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/b2eeb4acd21046d49691b95213d855c7...0f8bc160ff2840a890ec6b86ed74e1d5/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/48f389de751c4ffaa0f76dba16ccf1db...3f3551cc86e84164afdbb942c8a8995f/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12628:
URL: https://github.com/apache/arrow/pull/12628#issuecomment-1071243526
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #12628:
URL: https://github.com/apache/arrow/pull/12628#discussion_r828029071
##########
File path: cpp/src/arrow/flight/transport/grpc/grpc_client.cc
##########
@@ -316,7 +311,6 @@ class FinishableDataStream : public internal::ClientDataStream {
std::shared_ptr<ClientRpc> rpc_;
std::shared_ptr<Stream> stream_;
- std::shared_ptr<MemoryManager> memory_manager_;
Review comment:
It's already removed from the transport server implementations (the only reference there is to pass it on to the base class which does the heavy lifting)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12628:
URL: https://github.com/apache/arrow/pull/12628#issuecomment-1071243526
Benchmark runs are scheduled for baseline = 331fc4476b6c38c85d4403f1b9726111b4c28f26 and contender = 6a2c432ea715bd8f4975c4c3b303135030bfe3c2. 6a2c432ea715bd8f4975c4c3b303135030bfe3c2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/55b94f53ea87449083d6e7b67c8f3ca0...35c66407d39143d288b257bb2a5eb6a2/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/ecc3532adad14c65a763214a52a2ab5a...43e573230eb840c0849c5d81db34a861/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/b2eeb4acd21046d49691b95213d855c7...0f8bc160ff2840a890ec6b86ed74e1d5/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/48f389de751c4ffaa0f76dba16ccf1db...3f3551cc86e84164afdbb942c8a8995f/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12628:
URL: https://github.com/apache/arrow/pull/12628#issuecomment-1071243526
Benchmark runs are scheduled for baseline = 331fc4476b6c38c85d4403f1b9726111b4c28f26 and contender = 6a2c432ea715bd8f4975c4c3b303135030bfe3c2. 6a2c432ea715bd8f4975c4c3b303135030bfe3c2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/55b94f53ea87449083d6e7b67c8f3ca0...35c66407d39143d288b257bb2a5eb6a2/)
[Finished :arrow_down:0.13% :arrow_up:0.04%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/ecc3532adad14c65a763214a52a2ab5a...43e573230eb840c0849c5d81db34a861/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/b2eeb4acd21046d49691b95213d855c7...0f8bc160ff2840a890ec6b86ed74e1d5/)
[Finished :arrow_down:0.81% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/48f389de751c4ffaa0f76dba16ccf1db...3f3551cc86e84164afdbb942c8a8995f/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #12628:
URL: https://github.com/apache/arrow/pull/12628#discussion_r829177708
##########
File path: cpp/src/arrow/flight/test_definitions.cc
##########
@@ -1140,17 +1271,34 @@ void CudaDataTest::TestDoGet() {
FlightCallOptions options;
options.memory_manager = impl_->device->default_memory_manager();
+ const RecordBatchVector& batches =
+ reinterpret_cast<CudaTestServer*>(server_.get())->batches();
+
Ticket ticket{""};
std::unique_ptr<FlightStreamReader> stream;
ASSERT_OK(client_->DoGet(options, ticket, &stream));
- std::shared_ptr<Table> table;
- ASSERT_OK(stream->ReadAll(&table));
- for (const auto& column : table->columns()) {
- for (const auto& chunk : column->chunks()) {
- ASSERT_OK(CheckBuffersOnDevice(*chunk, *impl_->device));
+ size_t idx = 0;
+ while (true) {
+ FlightStreamChunk chunk;
+ ASSERT_OK(stream->Next(&chunk));
+ if (!chunk.data) break;
+
+ for (const auto& column : chunk.data->columns()) {
+ ASSERT_OK(CheckBuffersOnDevice(*column, *impl_->device));
Review comment:
Should we have a `CheckBuffersOnDevice` overload for record batches?
##########
File path: cpp/src/arrow/flight/test_definitions.cc
##########
@@ -518,22 +589,49 @@ void DataTest::TestIssue5095() {
//------------------------------------------------------------
// Specific tests for DoPut
+static constexpr char kExpectedMetadata[] = "foo bar";
+
class DoPutTestServer : public FlightServerBase {
public:
Status DoPut(const ServerCallContext& context,
std::unique_ptr<FlightMessageReader> reader,
std::unique_ptr<FlightMetadataWriter> writer) override {
descriptor_ = reader->descriptor();
+
+ if (descriptor_.type == FlightDescriptor::DescriptorType::CMD) {
+ if (descriptor_.cmd == "TestUndrained") {
+ // Don't read all the messages
+ return Status::OK();
+ }
+ }
+
int counter = 0;
+ FlightStreamChunk chunk;
while (true) {
- FlightStreamChunk chunk;
RETURN_NOT_OK(reader->Next(&chunk));
if (!chunk.data) break;
+ if (counter % 2 == 1) {
Review comment:
What happens if `counter % 2 == 0`? Should we assert that there's no app metadata?
##########
File path: cpp/src/arrow/flight/test_definitions.cc
##########
@@ -1140,17 +1271,34 @@ void CudaDataTest::TestDoGet() {
FlightCallOptions options;
options.memory_manager = impl_->device->default_memory_manager();
+ const RecordBatchVector& batches =
+ reinterpret_cast<CudaTestServer*>(server_.get())->batches();
Review comment:
Should we use `checked_cast` here?
##########
File path: cpp/src/arrow/flight/test_definitions.cc
##########
@@ -1175,6 +1323,23 @@ void CudaDataTest::TestDoPut() {
ASSERT_OK(writer->WriteRecordBatch(*cuda_batch));
}
ASSERT_OK(writer->Close());
+ ASSERT_OK(impl_->context->Synchronize());
+
+ const RecordBatchVector& written =
+ reinterpret_cast<CudaTestServer*>(server_.get())->batches();
Review comment:
`checked_cast` as well?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #12628:
URL: https://github.com/apache/arrow/pull/12628#discussion_r826969654
##########
File path: cpp/src/arrow/flight/test_definitions.cc
##########
@@ -91,6 +91,23 @@ void ConnectivityTest::TestShutdownWithDeadline() {
ASSERT_OK(server->Shutdown(&deadline));
ASSERT_OK(server->Wait());
}
+void ConnectivityTest::TestBrokenConnection() {
+ std::unique_ptr<FlightServerBase> server = ExampleTestServer();
+ ASSERT_OK_AND_ASSIGN(auto location, Location::ForScheme(transport(), "localhost", 0));
+ FlightServerOptions options(location);
+ ASSERT_OK(server->Init(options));
+
+ std::unique_ptr<FlightClient> client;
+ ASSERT_OK_AND_ASSIGN(location,
+ Location::ForScheme(transport(), "localhost", server->port()));
+ ASSERT_OK(FlightClient::Connect(location, &client));
+
+ ASSERT_OK(server->Shutdown());
+ ASSERT_OK(server->Wait());
+
+ std::unique_ptr<FlightInfo> info;
+ ASSERT_FALSE(client->GetFlightInfo(FlightDescriptor::Command(""), &info).ok());
Review comment:
Changed to test for IOError.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #12628:
URL: https://github.com/apache/arrow/pull/12628#discussion_r826918949
##########
File path: cpp/src/arrow/flight/flight_test.cc
##########
@@ -81,6 +81,7 @@ TEST_F(GrpcConnectivityTest, GetPort) { TestGetPort(); }
TEST_F(GrpcConnectivityTest, BuilderHook) { TestBuilderHook(); }
TEST_F(GrpcConnectivityTest, Shutdown) { TestShutdown(); }
TEST_F(GrpcConnectivityTest, ShutdownWithDeadline) { TestShutdownWithDeadline(); }
+TEST_F(GrpcConnectivityTest, BrokenConnection) { TestBrokenConnection(); }
Review comment:
Are these definitions meant to be repeated for each backend? If so, should there be a convenience macro so that none of them is forgotten?
##########
File path: cpp/src/arrow/flight/test_definitions.cc
##########
@@ -91,6 +91,23 @@ void ConnectivityTest::TestShutdownWithDeadline() {
ASSERT_OK(server->Shutdown(&deadline));
ASSERT_OK(server->Wait());
}
+void ConnectivityTest::TestBrokenConnection() {
+ std::unique_ptr<FlightServerBase> server = ExampleTestServer();
+ ASSERT_OK_AND_ASSIGN(auto location, Location::ForScheme(transport(), "localhost", 0));
+ FlightServerOptions options(location);
+ ASSERT_OK(server->Init(options));
+
+ std::unique_ptr<FlightClient> client;
+ ASSERT_OK_AND_ASSIGN(location,
+ Location::ForScheme(transport(), "localhost", server->port()));
+ ASSERT_OK(FlightClient::Connect(location, &client));
+
+ ASSERT_OK(server->Shutdown());
+ ASSERT_OK(server->Wait());
+
+ std::unique_ptr<FlightInfo> info;
+ ASSERT_FALSE(client->GetFlightInfo(FlightDescriptor::Command(""), &info).ok());
Review comment:
Should we test for a specific status code here (IOError perhaps)?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a change in pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on a change in pull request #12628:
URL: https://github.com/apache/arrow/pull/12628#discussion_r826547105
##########
File path: cpp/src/arrow/flight/transport/grpc/grpc_client.cc
##########
@@ -316,7 +311,6 @@ class FinishableDataStream : public internal::ClientDataStream {
std::shared_ptr<ClientRpc> rpc_;
std::shared_ptr<Stream> stream_;
- std::shared_ptr<MemoryManager> memory_manager_;
Review comment:
Is it possible to also remove `memory_manager_` from transport server implementations?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm closed pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
lidavidm closed pull request #12628:
URL: https://github.com/apache/arrow/pull/12628
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #12628:
URL: https://github.com/apache/arrow/pull/12628#discussion_r829177708
##########
File path: cpp/src/arrow/flight/test_definitions.cc
##########
@@ -1140,17 +1271,34 @@ void CudaDataTest::TestDoGet() {
FlightCallOptions options;
options.memory_manager = impl_->device->default_memory_manager();
+ const RecordBatchVector& batches =
+ reinterpret_cast<CudaTestServer*>(server_.get())->batches();
+
Ticket ticket{""};
std::unique_ptr<FlightStreamReader> stream;
ASSERT_OK(client_->DoGet(options, ticket, &stream));
- std::shared_ptr<Table> table;
- ASSERT_OK(stream->ReadAll(&table));
- for (const auto& column : table->columns()) {
- for (const auto& chunk : column->chunks()) {
- ASSERT_OK(CheckBuffersOnDevice(*chunk, *impl_->device));
+ size_t idx = 0;
+ while (true) {
+ FlightStreamChunk chunk;
+ ASSERT_OK(stream->Next(&chunk));
+ if (!chunk.data) break;
+
+ for (const auto& column : chunk.data->columns()) {
+ ASSERT_OK(CheckBuffersOnDevice(*column, *impl_->device));
Review comment:
Should we have a `CheckBuffersOnDevice` overload for record batches?
##########
File path: cpp/src/arrow/flight/test_definitions.cc
##########
@@ -518,22 +589,49 @@ void DataTest::TestIssue5095() {
//------------------------------------------------------------
// Specific tests for DoPut
+static constexpr char kExpectedMetadata[] = "foo bar";
+
class DoPutTestServer : public FlightServerBase {
public:
Status DoPut(const ServerCallContext& context,
std::unique_ptr<FlightMessageReader> reader,
std::unique_ptr<FlightMetadataWriter> writer) override {
descriptor_ = reader->descriptor();
+
+ if (descriptor_.type == FlightDescriptor::DescriptorType::CMD) {
+ if (descriptor_.cmd == "TestUndrained") {
+ // Don't read all the messages
+ return Status::OK();
+ }
+ }
+
int counter = 0;
+ FlightStreamChunk chunk;
while (true) {
- FlightStreamChunk chunk;
RETURN_NOT_OK(reader->Next(&chunk));
if (!chunk.data) break;
+ if (counter % 2 == 1) {
Review comment:
What happens if `counter % 2 == 0`? Should we assert that there's no app metadata?
##########
File path: cpp/src/arrow/flight/test_definitions.cc
##########
@@ -1140,17 +1271,34 @@ void CudaDataTest::TestDoGet() {
FlightCallOptions options;
options.memory_manager = impl_->device->default_memory_manager();
+ const RecordBatchVector& batches =
+ reinterpret_cast<CudaTestServer*>(server_.get())->batches();
Review comment:
Should we use `checked_cast` here?
##########
File path: cpp/src/arrow/flight/test_definitions.cc
##########
@@ -1175,6 +1323,23 @@ void CudaDataTest::TestDoPut() {
ASSERT_OK(writer->WriteRecordBatch(*cuda_batch));
}
ASSERT_OK(writer->Close());
+ ASSERT_OK(impl_->context->Synchronize());
+
+ const RecordBatchVector& written =
+ reinterpret_cast<CudaTestServer*>(server_.get())->batches();
Review comment:
`checked_cast` as well?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #12628: ARROW-15932: [C++][FlightRPC] Add more tests to the common Flight suite
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #12628:
URL: https://github.com/apache/arrow/pull/12628#issuecomment-1071243526
Benchmark runs are scheduled for baseline = 331fc4476b6c38c85d4403f1b9726111b4c28f26 and contender = 6a2c432ea715bd8f4975c4c3b303135030bfe3c2. 6a2c432ea715bd8f4975c4c3b303135030bfe3c2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/55b94f53ea87449083d6e7b67c8f3ca0...35c66407d39143d288b257bb2a5eb6a2/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/ecc3532adad14c65a763214a52a2ab5a...43e573230eb840c0849c5d81db34a861/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/b2eeb4acd21046d49691b95213d855c7...0f8bc160ff2840a890ec6b86ed74e1d5/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/48f389de751c4ffaa0f76dba16ccf1db...3f3551cc86e84164afdbb942c8a8995f/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org