Fix parallel warmup - Githubissues

shubhamtagra commented 4 years ago

Multiple fixes, reviewing by commits will be easier:

Move ReadRequestChain.call to long type: so that we dont restrict Chains to 2GB download limits
Make clusterType optional in CacheStatusRequest: this allows any client (like parallel warmup system), without the knowledge of cluster-type, to communicate with bookKeeper as long as bookKeeper has been initialized
Download from remote in chunks of max 10MB (configurable) during parallel warmup: This fixes heavy memory usage.
Align readRequests to block boundary: This fixes cases of wrong updates to cache leading to read errors
Use BookKeeper to create metadata-file instead of doing the work from outside
Fix Ranges in DownloadRequestContext to be closed-open instead of open-open: This improves merging of ranges
Avoid redundant warm-ups of blocks that have already been cached: This speeds up parallel warmup and also avoid unnecessary setAllCached requests to BookKeeper for blocks in cache
Fix wrong accounting of fileSize in cache when setAllCached is called with blocks already in cache. This, along with previous point, was leading to cache evictions due to SIZE even when cache was less than 20% used

shubhamtagra commented 4 years ago

fixes #361 along with other issues.

shubhamtagra commented 4 years ago

Tested this with sequential runs of query that reads ~200GB. Also tested with 20 parallel runs of same query.

shubhamtagra commented 4 years ago

Pushed a new commit that fixes #371, with that fix long running tests are green where multiple reads are issued in parallel and few are cancelled.

shubhamtagra commented 4 years ago

Unable to see this comment on github but i have tried to minimize the changes hence not moved things like these around.

On Wed, May 6, 2020 at 3:38 PM Harmandeep Singh notifications@github.com wrote:

@harmandeeps commented on this pull request.

In rubix-bookkeeper/src/test/java/com/qubole/rubix/bookkeeper/TestFileDownloader.java https://github.com/qubole/rubix/pull/368#discussion_r420679340:

@@ -150,7 +156,7 @@ public void testProcessDownloadRequests() throws Exception
 int dataDownloaded = (int) downloader.processDownloadRequests(requestChains);
int expectedDownloadedDataSize = 600;

int expectedDownloadedDataSize = 800;

nit: we can declare a static final cconstant for this.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/qubole/rubix/pull/368#pullrequestreview-406471635, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABYVF266CIPFOZVTX67SBFLRQEZKNANCNFSM4MVK6XZQ .

-- Thanks, Shubham Qubole, https://www.qubole.com

sopel39 commented 4 years ago

@stagraqubole when are you planning for next RubiX release so that we can update Presto dependencies?

shubhamtagra commented 4 years ago

@sopel39 its in progress, should be available in a day.

qubole / rubix

Fix parallel warmup #368

@harmandeeps commented on this pull request.