Fix multithread writing for fragment result cache by superqtqt · Pull Request #16546 · prestodb/presto

superqtqt · 2021-08-02T07:51:00Z

== RELEASE NOTES ==

General Changes
* Fix multithread writing for fragment result cache  #16455 
* add config `fragment-result-cache.max-single-pages-size` to control the max fragement cache file size

superqtqt · 2021-08-03T06:41:24Z

@shixuan-fan @kaikalur @highker @ajaygeorge @tdcmeehan
can you help me review the code

superqtqt · 2021-08-03T12:51:57Z

@shixuan-fan can you help me merge the code

shixuan-fan

Thanks for fixing this. Overall looks good just some nits.

presto-main/src/main/java/com/facebook/presto/operator/FileFragmentResultCacheManager.java

shixuan-fan · 2021-08-03T19:03:43Z

presto-main/src/main/java/com/facebook/presto/operator/FileFragmentResultCacheManager.java

Curious: Does it make sense that instead of creating PagesSerde every time, we create only once in the constructor, and directly replace current pagesSerde

Alternatively, you could use something similar to the changes I made in Trino trinodb/trino#5545 which (mostly) puts the not thread safe buffers into a context object that can be created / released. I say "mostly" here because I didn't move the Compressor instance in which you would need to do in order to use it that way.

Another thing worth pointing out is that this implementation, as written, doesn't support encryption of the serialized pages.

@shixuan-fan i have delete the write compress buffer and relation code.I find it not work.Can you help me approve

@pettyjamesm thanks for you advice .

@shixuan-fan i have delete the write compress buffer and relation code.I find it not work.Can you help me approve

I'm trying to understand this. Do you mean you tried @pettyjamesm 's suggestion but it does not work?

pettyjamesm · 2021-08-08T19:14:31Z

presto-main/src/main/java/com/facebook/presto/operator/FileFragmentResultCacheManager.java

You don’t want to catch Throwable here- since “should be fatal” members of the Error subheirarchy like OutOfMemoryError are not what you want to catch here. Seems like catching Exception would be sufficient instead.

Yes,I lack consideration.I hava fixed

Should we consider only extending it to RuntimeException? I don't think we should cover checked exception here.

Also, does it make sense to catch this separately from IOException and UncheckedIOException? There are legit reasons for them to happen, but maybe not for other exceptions?

shixuan-fan · 2021-08-09T22:35:43Z

presto-main/src/main/java/com/facebook/presto/operator/FileFragmentResultCacheManager.java

Should we consider only extending it to RuntimeException? I don't think we should cover checked exception here.

Also, does it make sense to catch this separately from IOException and UncheckedIOException? There are legit reasons for them to happen, but maybe not for other exceptions?

presto-main/src/main/java/com/facebook/presto/operator/FileFragmentResultCacheManager.java

shixuan-fan · 2021-08-10T00:47:44Z

Also, it might be great to add a test in TestFileFragmentResultCacheManager

shixuan-fan

Some nits on test. Mostly look good!

presto-main/src/main/java/com/facebook/presto/operator/FileFragmentResultCacheManager.java

shixuan-fan · 2021-08-10T16:50:36Z

presto-main/src/test/java/com/facebook/presto/operator/TestFileFragmentResultCacheManager.java

nit: threadCount. We don't use abbreviation in presto :)

i hava add comment
and delete threadCnt

shixuan-fan · 2021-08-10T16:51:10Z

presto-main/src/test/java/com/facebook/presto/operator/TestFileFragmentResultCacheManager.java

Let's remove this? It's useful for debugging but not useful as a unit test.

sorry ,i hava deleted

shixuan-fan · 2021-08-10T16:52:14Z

presto-main/src/test/java/com/facebook/presto/operator/TestFileFragmentResultCacheManager.java

How about just returning true and false?

shixuan-fan · 2021-08-10T16:53:49Z

presto-main/src/test/java/com/facebook/presto/operator/TestFileFragmentResultCacheManager.java

How about just checking if every future returns "success" (or true if applying previous comment). It might look neater :)

shixuan-fan · 2021-08-10T16:57:11Z

presto-main/src/test/java/com/facebook/presto/operator/TestFileFragmentResultCacheManager.java

Let's not move this method?

fix,thank you

shixuan-fan · 2021-08-10T16:58:24Z

presto-main/src/test/java/com/facebook/presto/operator/TestFileFragmentResultCacheManager.java

Let's create this as a local variable in the test method?

fix,thank you

shixuan-fan · 2021-08-10T16:59:21Z

presto-main/src/test/java/com/facebook/presto/operator/TestFileFragmentResultCacheManager.java

This is purely nit, but maybe for (int i = 0; i < 10; i++) would be clearer? This way we don't need to declare i and threadCnt outside of the loop.

shixuan-fan · 2021-08-10T17:01:41Z

presto-main/src/test/java/com/facebook/presto/operator/TestFileFragmentResultCacheManager.java

Curious: is %s,%s enough?

i name it test write content,thread %s,%s,that i think it more clear

superqtqt · 2021-08-11T06:56:59Z

thank you for reviewing my code.I am the first time to commit code ,which wasting your time.

shixuan-fan

LGTM.

No need to apologize, you are the one doing the hard work and helping fixing stuff. We really appreciate that :)

shixuan-fan · 2021-08-11T17:17:36Z

presto-main/src/test/java/com/facebook/presto/operator/TestFileFragmentResultCacheManager.java

Maybe revert the changes here and for SPLIT_2 if they are unintended.

shixuan-fan · 2021-08-11T22:28:59Z

Also, I'm wondering if we should change the commit and PR title to: "Fix multithread writing for fragment result cache"

superqtqt · 2021-08-12T02:03:02Z

Also, I'm wondering if we should change the commit and PR title to: "Fix multithread writing for fragment result cache"

ok ,i have changed

shixuan-fan · 2021-08-12T19:44:58Z

Merged. Thanks for the help identifying/fixing bugs :)

superqtqt force-pushed the fix/fragmentCacheWriteBug branch 2 times, most recently from 4df400f to 3dbb9c0 Compare August 3, 2021 02:14

tdcmeehan requested a review from shixuan-fan August 3, 2021 14:48

shixuan-fan requested changes Aug 3, 2021

View reviewed changes

superqtqt force-pushed the fix/fragmentCacheWriteBug branch 3 times, most recently from 8807909 to c31a85c Compare August 6, 2021 09:00

pettyjamesm reviewed Aug 8, 2021

View reviewed changes

superqtqt force-pushed the fix/fragmentCacheWriteBug branch from c31a85c to 1b51083 Compare August 9, 2021 01:34

shixuan-fan reviewed Aug 9, 2021

View reviewed changes

superqtqt force-pushed the fix/fragmentCacheWriteBug branch 2 times, most recently from 936a293 to 2bf3c72 Compare August 10, 2021 13:10

shixuan-fan reviewed Aug 10, 2021

View reviewed changes

superqtqt force-pushed the fix/fragmentCacheWriteBug branch from 2bf3c72 to a1ab2db Compare August 11, 2021 06:44

superqtqt force-pushed the fix/fragmentCacheWriteBug branch from a1ab2db to 5c0f19f Compare August 11, 2021 07:09

shixuan-fan approved these changes Aug 11, 2021

View reviewed changes

superqtqt changed the title ~~fix write cache bug when multithreading writing~~ Fix multithread writing for fragment result cache Aug 12, 2021

Fix multithread writing for fragment result cache

5176668

superqtqt force-pushed the fix/fragmentCacheWriteBug branch from 5c0f19f to 5176668 Compare August 12, 2021 02:02

shixuan-fan merged commit fda59c3 into prestodb:master Aug 12, 2021

varungajjala mentioned this pull request Aug 16, 2021

Add release notes for 0.260 #16619

Merged

3 tasks

Conversation

superqtqt commented Aug 2, 2021 • edited by ajaygeorge Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

superqtqt commented Aug 3, 2021

Uh oh!

superqtqt commented Aug 3, 2021

Uh oh!

shixuan-fan left a comment

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!

superqtqt Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pettyjamesm Aug 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

shixuan-fan commented Aug 10, 2021

Uh oh!

shixuan-fan left a comment

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!

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!

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!

superqtqt commented Aug 11, 2021

Uh oh!

shixuan-fan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

superqtqt commented Aug 2, 2021 •

edited by ajaygeorge

Loading

superqtqt Aug 4, 2021 •

edited

Loading

pettyjamesm Aug 8, 2021 •

edited

Loading