Skip to content

PHOENIX-6436 OrderedResultIterator overestimates memory requirements. - #1189

Merged
lhofhansl merged 1 commit into
apache:masterfrom
lhofhansl:6436
Apr 5, 2021
Merged

PHOENIX-6436 OrderedResultIterator overestimates memory requirements.#1189
lhofhansl merged 1 commit into
apache:masterfrom
lhofhansl:6436

Conversation

@lhofhansl

Copy link
Copy Markdown
Contributor

See Jira.

the OrderedResultIterator will store topN values in either a BufferedQueue or a SizeBoundQueue. Each limit the memory used to the passed thresholdBytes (default is 20MB), the BufferQueue will spool to disk when reaching that size, SizeBoundQueue will fail.

Hence we additionally limit the worst case memory consumption to thresholdBytes.

I noticed this when implementing limit and topN pushdown for the Trino Phoenix connector: trinodb/trino#7490

@lhofhansl

lhofhansl commented Apr 4, 2021

Copy link
Copy Markdown
ContributorAuthor

It turns out that for large sets Trino is actually performing the topN faster, even though it has to pull in all the data. This is due to the final client merge work that Phoenix is doing.
So it's less important now to fix this here, since I'm limiting topN pushdown in Trino now. Still wrong, though.

@stoty

stoty commented Apr 5, 2021

Copy link
Copy Markdown
Contributor

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec5m 34sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+1 💚hbaseanti0m 0sPatch does not have any anti-patterns.
+1 💚@author0m 0sThe patch does not contain any @author tags.
-1 ❌test4tests0m 0sThe patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
_ master Compile Tests _
+1 💚mvninstall16m 3smaster passed
+0hbaserecompile22m 25sHBase recompiled.
+1 💚compile1m 1smaster passed
+1 💚checkstyle0m 38smaster passed
+1 💚javadoc0m 52smaster passed
+0 🆗spotbugs3m 4sphoenix-core in master has 956 extant spotbugs warnings.
_ Patch Compile Tests _
+1 💚mvninstall8m 6sthe patch passed
+0hbaserecompile18m 42sHBase recompiled.
+1 💚compile1m 3sthe patch passed
+1 💚javac1m 3sthe patch passed
-1 ❌checkstyle0m 37sphoenix-core: The patch generated 3 new + 193 unchanged - 2 fixed = 196 total (was 195)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚javadoc0m 51sthe patch passed
+1 💚spotbugs3m 16sthe patch passed
_ Other Tests _
-1 ❌unit107m 44sphoenix-core in the patch failed.
+1 💚asflicense0m 45sThe patch does not generate ASF License warnings.
166m 45s
ReasonTests
Failed junit testsphoenix.end2end.PermissionNSEnabledWithCustomAccessControllerIT
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1189/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#1189
Optional Testsdupname asflicense javac javadoc unit spotbugs hbaserebuild hbaseanti checkstyle compile
unameLinux 328f92a531e7 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev/phoenix-personality.sh
git revisionmaster / 6e1b35d
Default JavaPrivate Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08
checkstylehttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1189/1/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt
unithttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1189/1/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt
Test Resultshttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1189/1/testReport/
Max. process+thread count14326 (vs. ulimit of 30000)
modulesC: phoenix-core U: phoenix-core
Console outputhttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1189/1/console
versionsgit=2.7.4 maven=3.3.9 spotbugs=4.1.3
Powered byApache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@gjacoby126gjacoby126 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, thanks @lhofhansl

@lhofhansl
lhofhansl merged commit 21599ac into apache:masterApr 5, 2021
@lhofhansl

Copy link
Copy Markdown
ContributorAuthor

Thanks @gjacoby126

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@lhofhansl@stoty@gjacoby126