[GitHub] [arrow] pitrou commented on a diff in pull request #14257: ARROW-17853: [Python][CI] Timeout in test_dataset.py::test_write_dataset_s3_put_only

[GitBox <gi...@apache.org>]

pitrou commented on code in PR #14257:
URL: https://github.com/apache/arrow/pull/14257#discussion_r988621032


##########
cpp/src/arrow/dataset/dataset_writer_test.cc:
##########
@@ -234,6 +234,18 @@ TEST_F(DatasetWriterTestFixture, BasicFileDirectoryPrefix) {
   AssertFilesCreated({"testdir/a/1_chunk-0.arrow"});
 }
 
+TEST_F(DatasetWriterTestFixture, DirectoryCreateFails) {
+  // This should fail to be created
+  write_options_.base_dir = "///doesnotexist";
+  EXPECT_OK_AND_ASSIGN(auto dataset_writer,
+                       DatasetWriter::Make(write_options_, scheduler_.get()));
+  Future<> queue_fut = dataset_writer->WriteRecordBatch(MakeBatch(100), "a", "1_");
+  AssertFinished(queue_fut);
+  ASSERT_OK(dataset_writer->Finish());
+  scheduler_->End();
+  ASSERT_FINISHES_AND_RAISES(Invalid, scheduler_->OnFinished());

Review Comment:
   Hmm... so the only way to know that writing failed is to end the scheduler? Why isn't it popped up earlier?



##########
cpp/src/arrow/util/async_util.h:
##########
@@ -311,6 +306,8 @@ class ARROW_EXPORT AsyncTaskScheduler {
   ///        The default (nullptr) will use a FIFO queue if there is a throttle.
   static std::unique_ptr<AsyncTaskScheduler> Make(Throttle* throttle = NULLPTR,
                                                   std::unique_ptr<Queue> queue = NULLPTR);
+
+  virtual bool IsEnded() = 0;

Review Comment:
   +1 for const and a docstring (also stating the potential race condition in usage)



##########
cpp/src/arrow/util/async_util.cc:
##########
@@ -98,13 +98,53 @@ class FifoQueue : public AsyncTaskScheduler::Queue {
   std::list<std::unique_ptr<Task>> tasks_;
 };
 
+class AlreadyFailedScheduler : public AsyncTaskScheduler {
+ public:
+  explicit AlreadyFailedScheduler(Status failure_reason,
+                                  FnOnce<Status(Status)> finish_callback)
+      : failure_reason_(std::move(failure_reason)),
+        finish_callback_(std::move(finish_callback)) {}
+  bool AddTask(std::unique_ptr<Task> task) override { return false; }
+  void End() override {
+    std::ignore = std::move(finish_callback_)(failure_reason_);
+    self.reset();
+  }
+  Future<> OnFinished() const override {
+    DCHECK(false) << "You should not rely on sub-scheduler's OnFinished.  Use a "
+                     "finished callback when creating the sub-scheduler instead";
+    return Future<>::MakeFinished(Status::UnknownError("Unreachable code encountered"));

Review Comment:
   Can use `Status::Abort` perhaps instead.



-- 
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