Do not throw from PartitionedOutputBufferManager::getData by karteekmurthys · Pull Request #3502 · facebookincubator/velox

karteekmurthys · 2022-12-13T04:36:16Z

A task being executed by multiple drivers, and if one of the drivers catches an
exception, can terminate the task. This termination will remove any
PartitionedOutputBuffer assigned to that taskId. In a parallel thread, the task
manager may try to access this buffer leading to "Output buffers not found"
exception. This exception is valid, but instead of the exception encountered by
the Driver trickling up to the user, the "Output buffers not found" exception
may sometimes get thrown to the user.

Instead of throwing exception, the getData() will return
a bool indicating whether a matching buffer for a given task id was found or not.

See #3009 for more context.

netlify · 2022-12-13T04:36:20Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`29b779f`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/639cc5c531a0f10009f78d09

mbasmanova · 2022-12-13T16:08:01Z

velox/exec/PartitionedOutputBufferManager.cpp

@karteekmurthys

There are multiple places that call getBuffer. It is not clear why this particular place should handle the exception and others should not. Would you clarify?

Similar to the other PR, we should avoid programming-by-exception pattern and instead introduce a method getBufferIfExists that return nullptr if buffer doesn't exist. This method can then be used whenever the caller wants to handle missing buffer (i.e. deleteResults and maybe here).

The method documentation needs to be updated to describe the new behavior and a test needs to be added.

mbasmanova

per comments above

… caller

mbasmanova

@karteekmurthys Karteek, thank you for iterating on this PR. The PR description explains the problem well. Thank you for taking the time to write it up.

This is misleading and must be handled correctly.

Agree, but would you elaborate on what does it mean to "handled correctly"? In other words, would you update the description to explain the solution implemented here?

Some comments below.

I'm seeing PartitionedOutputBufferManagerTest crashing. Please, take a look.

mbasmanova · 2022-12-15T11:35:05Z

velox/exec/PartitionedOutputBufferManager.h

  // Retrieves the set of buffers for a query.
  std::shared_ptr<PartitionedOutputBuffer> getBuffer(const std::string& taskId);
-
+  std::shared_ptr<PartitionedOutputBuffer> getBufferIfExists(


This is a nice method. Please, add empty line before this method and document it. Let's use this method in PartitionedOutputBufferManager::deleteResults as well.

mbasmanova · 2022-12-15T11:35:32Z

velox/exec/PartitionedOutputBufferManager.h

@@ -265,7 +284,8 @@ class PartitionedOutputBufferManager {
 private:
  // Retrieves the set of buffers for a query.


Let's clarify that this method throws is buffer for the specified task does not exist.

I still think we should clarify that this method throws when task is not found.

// Retrieves the set of buffers for the specified task. Throws buffers-no-found exception if not found.

mbasmanova · 2022-12-15T11:36:30Z

velox/exec/PartitionedOutputBufferManager.h

-// BufferManager::acknowledge.
-using DataAvailableCallback = std::function<
-    void(std::vector<std::unique_ptr<folly::IOBuf>> pages, int64_t sequence)>;
+enum GetDataStatus { SUCCESS, ERR_BUFFER_NOT_FOUND };


Use enum class and kXxx naming for the values. See https://github.com/facebookincubator/velox/blob/main/CODING_STYLE.md

velox/exec/PartitionedOutputBufferManager.h

mbasmanova · 2022-12-15T11:41:01Z

velox/exec/PartitionedOutputBufferManager.cpp

+  }
+  std::vector<std::unique_ptr<folly::IOBuf>> pages;
+  auto param = std::make_unique<DataAvailableCallbackParam>(
+      std::move(pages), sequence, ERR_BUFFER_NOT_FOUND);


It would be simpler to just return 'false' to indicate that buffers for the task do not exist and therefore the 'notify' callback cannot be queued. See my other comment.

mbasmanova · 2022-12-15T11:42:40Z

velox/exec/tests/PartitionedOutputBufferManagerTest.cpp

  }
 }
+
+TEST_F(PartitionedOutputBufferManagerTest, getDataOnTaskWithNoBuffer) {


nit: perhaps, rename to getDataOnFailedTask and add comments to explain the scenario this test is verifying.

facebook-github-bot · 2022-12-16T18:02:26Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mbasmanova

@karteekmurthys Looks good to me % a few small comments and PR description needing an update to explain the solution implemented here.

mbasmanova · 2022-12-16T18:25:45Z

velox/exec/PartitionedOutputBufferManager.cpp

-        }
-        return it->second;
-      });
+  auto buffer = getBufferIfExists(taskId);


nit: a cleaner way to write this is

if (auto buffer = getBufferIfExists(taskId)) { buffer->deleteResults(destination); }

mbasmanova · 2022-12-16T18:25:52Z

velox/exec/PartitionedOutputBufferManager.cpp

    int64_t sequence,
    DataAvailableCallback notify) {
-  getBuffer(taskId)->getData(destination, maxBytes, sequence, notify);
+  auto buffer = getBufferIfExists(taskId);


mbasmanova · 2022-12-16T18:26:27Z

velox/exec/PartitionedOutputBufferManager.h

  // Retrieves the set of buffers for a query.
  std::shared_ptr<PartitionedOutputBuffer> getBuffer(const std::string& taskId);

+  // Retrieves the set of buffers for a query if exists. If taskId is not found,


query -> task

"If taskId is not found," -> "Returns null if task not found."

mbasmanova · 2022-12-16T18:28:07Z

velox/exec/PartitionedOutputBufferManager.h

@@ -265,7 +284,8 @@ class PartitionedOutputBufferManager {
 private:
  // Retrieves the set of buffers for a query.


I still think we should clarify that this method throws when task is not found.

// Retrieves the set of buffers for the specified task. Throws buffers-no-found exception if not found.

mbasmanova · 2022-12-16T18:28:55Z

velox/exec/tests/PartitionedOutputBufferManagerTest.cpp

  }
 }
+
+TEST_F(PartitionedOutputBufferManagerTest, getDataOnFailedTask) {


Let's update existing calls to getData to assert that they return true.

mbasmanova · 2022-12-16T18:29:12Z

velox/exec/tests/PartitionedOutputBufferManagerTest.cpp

+      1,
+      [&notified](
+          std::vector<std::unique_ptr<folly::IOBuf>> pages, int64_t sequence) {
+        notified = true;


Let's simply this using VELOX_UNREACHABLE();

mbasmanova · 2022-12-16T18:29:30Z

velox/exec/tests/PartitionedOutputBufferManagerTest.cpp

+  // manager or was removed by a parallel thread must return false. The `notify`
+  // callback must not be registered.
+  bool notified = false;
+  auto ret = bufferManager_->getData(


ret -> found

mbasmanova

@karteekmurthys Thank you, Karteek.

facebook-github-bot · 2022-12-16T19:54:54Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-12-16T19:57:15Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-12-19T17:09:23Z

@mbasmanova merged this pull request in a91b227.

karteekmurthys self-assigned this Dec 13, 2022

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 13, 2022

karteekmurthys force-pushed the output-buffer-not-found branch 2 times, most recently from c93639f to baadf75 Compare December 13, 2022 06:34

karteekmurthys requested review from aditi-pandit and mbasmanova December 13, 2022 07:38

mbasmanova reviewed Dec 13, 2022

View reviewed changes

mbasmanova requested changes Dec 13, 2022

View reviewed changes

PartitionedOutputBufferManager::getData, handle exceptions and notify…

4391bcf

… caller

karteekmurthys force-pushed the output-buffer-not-found branch from cb8096f to 8634093 Compare December 15, 2022 07:41

Addressed comments

9fd12d4

karteekmurthys force-pushed the output-buffer-not-found branch from 8634093 to 9fd12d4 Compare December 15, 2022 07:46

mbasmanova requested changes Dec 15, 2022

View reviewed changes

karteekmurthys force-pushed the output-buffer-not-found branch from 7444ed7 to e19b3d6 Compare December 16, 2022 17:55

Reverted callback changes, retained older interface

28f295c

karteekmurthys force-pushed the output-buffer-not-found branch from e19b3d6 to 28f295c Compare December 16, 2022 17:57

mbasmanova reviewed Dec 16, 2022

View reviewed changes

Addressed comments

29b779f

mbasmanova approved these changes Dec 16, 2022

View reviewed changes

mbasmanova changed the title ~~PartitionedOutputBufferManager::getData notify caller on exception~~ Do not throw from PartitionedOutputBufferManager::getData Dec 16, 2022

facebook-github-bot closed this in a91b227 Dec 19, 2022

facebook-github-bot added the Merged label Dec 19, 2022

This was referenced Dec 20, 2022

Advance Velox prestodb/presto#18827

Merged

[Native] Fix race condition that throws "Output buffers not found" prestodb/presto#18776

Merged

karteekmurthys mentioned this pull request Dec 22, 2022

Fix possible race condition that throws Output Buffer not found #3593

Closed

marin-ma pushed a commit to marin-ma/velox-oap that referenced this pull request Dec 15, 2023

[VL] Fix native cmake include path (facebookincubator#3502)

dd7eab4

		@@ -265,7 +284,8 @@ class PartitionedOutputBufferManager {
		private:
		// Retrieves the set of buffers for a query.

Conversation

karteekmurthys commented Dec 13, 2022 • edited by mbasmanova Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Dec 16, 2022

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Dec 16, 2022

Uh oh!

facebook-github-bot commented Dec 16, 2022

Uh oh!

facebook-github-bot commented Dec 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

karteekmurthys commented Dec 13, 2022 •

edited by mbasmanova

Loading

netlify bot commented Dec 13, 2022 •

edited

Loading