Do not fail getData for deleted buffer #9207
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
|
Currently impacts Prestissimo: prestodb/presto#22129 |
arhimondr
force-pushed
the
fetch_after_delete
branch
from
c8f85ff
to
2e4c1a5
Compare
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| message(STATUS "Using Boost - Bundled") |
There was a problem hiding this comment.
Why is this change? CC: @kgpai @assignUser @majetideepak
There was a problem hiding this comment.
Good catch, thanks! This is a generated file and shouldn't be checked in. (it's in .gitignore too)
| updateAfterAcknowledgeLocked(freed, promises); | ||
| data = buffer->getData( | ||
| maxBytes, sequence, notify, activeCheck, arbitraryBuffer_.get()); | ||
| if (buffer) { |
There was a problem hiding this comment.
Let's update comments for this method in the header file to mention this behavior.
There was a problem hiding this comment.
Updated the comment for deleteResults
There was a problem hiding this comment.
Thanks. Would you also update comments for OutputBufferManager::getData API as this is the public API used by Prestissimo?
|
@arhimondr Also we never saw it happening in shadow workload, could it be something not set up correctly in the tests? |
@Yuhta We are seeing that: 20240324_170232_00988_y57ss |
arhimondr
force-pushed
the
fetch_after_delete
branch
from
2e4c1a5
to
eba2d11
Compare
|
Sorry for the delay. Updated. @mbasmanova, @Yuhta could you please take an another look? |
@Yuhta This happens ~10 times per day. Not frequent, but its there since the beginning of Prestissimo and a major source of flakiness. |
There was a problem hiding this comment.
@arhimondr Thank you for the fix. Looks good % one ask for a comment.
Since requests can arrive out of order
arhimondr
force-pushed
the
fetch_after_delete
branch
from
eba2d11
to
13e1ce4
Compare
|
Test failure related to #9318 |
|
@arhimondr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
We are seeing this as well internally. @arhimondr thanks for the fix! |
|
@arhimondr merged this pull request in d4b3618. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Summary: Sometimes requests can arrive out of order. When a `getData` arrives after a buffer is closed it may cause an unwanted failure. Fixes prestodb/presto#22129 Pull Request resolved: facebookincubator#9207 Reviewed By: mbasmanova Differential Revision: D55591528 Pulled By: arhimondr fbshipit-source-id: db493477e236b6c25ea39887a3bab5273558455b
Sometimes requests can arrive out of order. When a
getDataarrives after a buffer is closed it may cause an unwanted failure.Fixes prestodb/presto#22129