Skip to content

PHOENIX-6557 Fix code problems flagged by SpotBugs as High priority - #1319

Closed
stoty wants to merge 2 commits into
apache:masterfrom
stoty:PHOENIX-6557
Closed

PHOENIX-6557 Fix code problems flagged by SpotBugs as High priority#1319
stoty wants to merge 2 commits into
apache:masterfrom
stoty:PHOENIX-6557

Conversation

@stoty

@stotystoty commented Sep 21, 2021

Copy link
Copy Markdown
Contributor

A lot of this patch is setting the character sets, and boxing/unboxing optimizations, but there are a handful of actual bug fixes hidden in there.

@stoty
stoty requested review from ChinmaySKulkarni, apurtell, gjacoby126 and virajjasani and removed request for apurtell and gjacoby126September 21, 2021 13:19
virajjasani
virajjasani reviewed Sep 21, 2021
View reviewed changes
Comment threadsrc/main/config/spotbugs/spotbugs-exclude.xml
<Class name="~org\.apache\.phoenix\.coprocessor\.generated\..*" />
</Match>
<Match>
<BugPattern name="NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE" />

@virajjasanivirajjasaniSep 21, 2021

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.

Good to comment what we are excluding here, just a short text is fine

@stotystotySep 27, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done.
Also removed the synamic SQL Statement exlcusion, and opened PHOENIX-6560 as a follow-up

@stoty

stoty commented Sep 21, 2021

Copy link
Copy Markdown
ContributorAuthor

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec6m 34sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 1sNo 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 _
+0 🆗mvndep4m 57sMaven dependency ordering for branch
+1 💚mvninstall19m 31smaster passed
+0hbaserecompile32m 25sHBase recompiled.
+1 💚compile1m 46smaster passed
+1 💚checkstyle3m 59smaster passed
+1 💚javadoc2m 5smaster passed
+0 🆗spotbugs5m 0sroot in master has 1031 extant spotbugs warnings.
+0 🆗spotbugs3m 24sphoenix-core in master has 972 extant spotbugs warnings.
_ Patch Compile Tests _
+0 🆗mvndep0m 15sMaven dependency ordering for patch
+1 💚mvninstall18m 9sthe patch passed
+0hbaserecompile26m 43sHBase recompiled.
+1 💚compile1m 59sthe patch passed
+1 💚javac1m 59sthe patch passed
-1 ❌checkstyle3m 56sroot: The patch generated 372 new + 9895 unchanged - 369 fixed = 10267 total (was 10264)
-1 ❌whitespace0m 0sThe patch has 2 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
-1 ❌whitespace0m 0sThe patch 1 line(s) with tabs.
+1 💚xml0m 3sThe patch has no ill-formed XML file.
+1 💚javadoc2m 10sthe patch passed
+1 💚spotbugs3m 23sphoenix-core generated 0 new + 704 unchanged - 268 fixed = 704 total (was 972)
+1 💚spotbugs5m 12sroot generated 0 new + 757 unchanged - 274 fixed = 757 total (was 1031)
_ Other Tests _
-1 ❌unit2m 6sroot in the patch failed.
-1 ❌asflicense0m 37sThe patch generated 613 ASF License warnings.
103m 52s
ReasonTests
Failed junit testsphoenix.mapreduce.FormatToBytesWritableMapperTest
phoenix.mapreduce.util.ColumnInfoToStringEncoderDecoderTest
phoenix.util.ColumnInfoTest
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#1319
Optional Testsdupname asflicense javac javadoc unit spotbugs hbaserebuild hbaseanti checkstyle compile xml
unameLinux 7f3d6fdb319a 4.15.0-153-generic #160-Ubuntu SMP Thu Jul 29 06:54:29 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev/phoenix-personality.sh
git revisionmaster / c1ffb0b
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-1319/1/artifact/yetus-general-check/output/diff-checkstyle-root.txt
whitespacehttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/1/artifact/yetus-general-check/output/whitespace-eol.txt
whitespacehttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/1/artifact/yetus-general-check/output/whitespace-tabs.txt
unithttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/1/artifact/yetus-general-check/output/patch-unit-root.txt
Test Resultshttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/1/testReport/
asflicensehttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/1/artifact/yetus-general-check/output/patch-asflicense-problems.txt
Max. process+thread count495 (vs. ulimit of 30000)
modulesC: phoenix-core . U: .
Console outputhttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/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.

@stoty

stoty commented Sep 21, 2021

Copy link
Copy Markdown
ContributorAuthor

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 7sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 1sNo 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 _
+0 🆗mvndep5m 15sMaven dependency ordering for branch
+1 💚mvninstall19m 45smaster passed
+0hbaserecompile31m 57sHBase recompiled.
+1 💚compile1m 47smaster passed
+1 💚checkstyle3m 56smaster passed
+1 💚javadoc2m 12smaster passed
+0 🆗spotbugs4m 59sroot in master has 1031 extant spotbugs warnings.
+0 🆗spotbugs3m 22sphoenix-core in master has 972 extant spotbugs warnings.
_ Patch Compile Tests _
+0 🆗mvndep0m 16sMaven dependency ordering for patch
+1 💚mvninstall16m 53sthe patch passed
+0hbaserecompile23m 58sHBase recompiled.
+1 💚compile1m 49sthe patch passed
+1 💚javac1m 49sthe patch passed
-1 ❌checkstyle4m 1sroot: The patch generated 372 new + 9895 unchanged - 369 fixed = 10267 total (was 10264)
-1 ❌whitespace0m 0sThe patch has 2 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
-1 ❌whitespace0m 1sThe patch 1 line(s) with tabs.
+1 💚xml0m 3sThe patch has no ill-formed XML file.
+1 💚javadoc2m 5sthe patch passed
+1 💚spotbugs3m 19sphoenix-core generated 0 new + 704 unchanged - 268 fixed = 704 total (was 972)
+1 💚spotbugs4m 59sroot generated 0 new + 757 unchanged - 274 fixed = 757 total (was 1031)
_ Other Tests _
-1 ❌unit2m 3sroot in the patch failed.
-1 ❌asflicense0m 36sThe patch generated 613 ASF License warnings.
94m 44s
ReasonTests
Failed junit testsphoenix.mapreduce.FormatToBytesWritableMapperTest
phoenix.mapreduce.util.ColumnInfoToStringEncoderDecoderTest
phoenix.util.ColumnInfoTest
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#1319
Optional Testsdupname asflicense javac javadoc unit spotbugs hbaserebuild hbaseanti checkstyle compile xml
unameLinux 741eb6fee043 4.15.0-153-generic #160-Ubuntu SMP Thu Jul 29 06:54:29 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev/phoenix-personality.sh
git revisionmaster / c1ffb0b
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-1319/2/artifact/yetus-general-check/output/diff-checkstyle-root.txt
whitespacehttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/2/artifact/yetus-general-check/output/whitespace-eol.txt
whitespacehttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/2/artifact/yetus-general-check/output/whitespace-tabs.txt
unithttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/2/artifact/yetus-general-check/output/patch-unit-root.txt
Test Resultshttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/2/testReport/
asflicensehttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/2/artifact/yetus-general-check/output/patch-asflicense-problems.txt
Max. process+thread count493 (vs. ulimit of 30000)
modulesC: phoenix-core . U: .
Console outputhttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/2/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.

@stoty

stoty commented Sep 21, 2021

Copy link
Copy Markdown
ContributorAuthor

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec1m 4sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 1sNo 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 _
+0 🆗mvndep5m 3sMaven dependency ordering for branch
+1 💚mvninstall19m 35smaster passed
+0hbaserecompile31m 37sHBase recompiled.
+1 💚compile1m 49smaster passed
+1 💚checkstyle4m 1smaster passed
+1 💚javadoc2m 7smaster passed
+0 🆗spotbugs4m 56sroot in master has 1031 extant spotbugs warnings.
+0 🆗spotbugs3m 18sphoenix-core in master has 972 extant spotbugs warnings.
_ Patch Compile Tests _
+0 🆗mvndep0m 15sMaven dependency ordering for patch
+1 💚mvninstall16m 59sthe patch passed
+0hbaserecompile24m 4sHBase recompiled.
+1 💚compile1m 47sthe patch passed
+1 💚javac1m 47sthe patch passed
-1 ❌checkstyle4m 3sroot: The patch generated 370 new + 9895 unchanged - 369 fixed = 10265 total (was 10264)
-1 ❌whitespace0m 0sThe patch 1 line(s) with tabs.
+1 💚xml0m 2sThe patch has no ill-formed XML file.
+1 💚javadoc2m 7sthe patch passed
+1 💚spotbugs3m 21sphoenix-core generated 0 new + 704 unchanged - 268 fixed = 704 total (was 972)
+1 💚spotbugs5m 0sroot generated 0 new + 757 unchanged - 274 fixed = 757 total (was 1031)
_ Other Tests _
-1 ❌unit118m 1sroot in the patch failed.
-1 ❌asflicense1m 30sThe patch generated 613 ASF License warnings.
212m 38s
ReasonTests
Failed junit testsphoenix.end2end.index.MutableIndexFailureIT
phoenix.end2end.CSVCommonsLoaderIT
phoenix.end2end.VariableLengthPKIT
phoenix.end2end.CsvBulkLoadToolIT
phoenix.end2end.ArithmeticQueryIT
phoenix.end2end.index.MutableIndexFailureWithNamespaceIT
phoenix.end2end.index.IndexMetadataIT
phoenix.end2end.TenantSpecificTablesDDLIT
phoenix.end2end.OrphanViewToolIT
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/3/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#1319
Optional Testsdupname asflicense javac javadoc unit spotbugs hbaserebuild hbaseanti checkstyle compile xml
unameLinux ad7e8bd10000 4.15.0-153-generic #160-Ubuntu SMP Thu Jul 29 06:54:29 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev/phoenix-personality.sh
git revisionmaster / c1ffb0b
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-1319/3/artifact/yetus-general-check/output/diff-checkstyle-root.txt
whitespacehttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/3/artifact/yetus-general-check/output/whitespace-tabs.txt
unithttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/3/artifact/yetus-general-check/output/patch-unit-root.txt
Test Resultshttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/3/testReport/
asflicensehttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/3/artifact/yetus-general-check/output/patch-asflicense-problems.txt
Max. process+thread count8314 (vs. ulimit of 30000)
modulesC: phoenix-core . U: .
Console outputhttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/3/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.

@stoty

stoty commented Sep 22, 2021

Copy link
Copy Markdown
ContributorAuthor

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec5m 28sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 2sNo 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 _
+0 🆗mvndep5m 18sMaven dependency ordering for branch
+1 💚mvninstall16m 27smaster passed
+0hbaserecompile28m 2sHBase recompiled.
+1 💚compile1m 39smaster passed
+1 💚checkstyle5m 27smaster passed
+1 💚javadoc2m 5smaster passed
+0 🆗spotbugs4m 35sroot in master has 1031 extant spotbugs warnings.
+0 🆗spotbugs3m 2sphoenix-core in master has 972 extant spotbugs warnings.
_ Patch Compile Tests _
+0 🆗mvndep0m 16sMaven dependency ordering for patch
+1 💚mvninstall13m 41sthe patch passed
+0hbaserecompile20m 18sHBase recompiled.
+1 💚compile1m 39sthe patch passed
+1 💚javac1m 39sthe patch passed
-1 ❌checkstyle5m 31sroot: The patch generated 369 new + 9898 unchanged - 369 fixed = 10267 total (was 10267)
-1 ❌whitespace0m 0sThe patch 1 line(s) with tabs.
+1 💚xml0m 3sThe patch has no ill-formed XML file.
+1 💚javadoc2m 3sthe patch passed
+1 💚spotbugs3m 3sphoenix-core generated 0 new + 704 unchanged - 268 fixed = 704 total (was 972)
+1 💚spotbugs4m 32sroot generated 0 new + 757 unchanged - 274 fixed = 757 total (was 1031)
_ Other Tests _
-1 ❌unit114m 27sroot in the patch failed.
-1 ❌asflicense1m 50sThe patch generated 613 ASF License warnings.
207m 52s
ReasonTests
Failed junit testsphoenix.end2end.index.IndexMetadataIT
phoenix.end2end.index.MutableIndexFailureWithNamespaceIT
phoenix.end2end.CsvBulkLoadToolIT
phoenix.end2end.index.MutableIndexFailureIT
phoenix.end2end.TenantSpecificTablesDDLIT
phoenix.end2end.VariableLengthPKIT
phoenix.end2end.CSVCommonsLoaderIT
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/4/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#1319
Optional Testsdupname asflicense javac javadoc unit spotbugs hbaserebuild hbaseanti checkstyle compile xml
unameLinux bc0706fd6feb 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 / c1ffb0b
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-1319/4/artifact/yetus-general-check/output/diff-checkstyle-root.txt
whitespacehttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/4/artifact/yetus-general-check/output/whitespace-tabs.txt
unithttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/4/artifact/yetus-general-check/output/patch-unit-root.txt
Test Resultshttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/4/testReport/
asflicensehttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/4/artifact/yetus-general-check/output/patch-asflicense-problems.txt
Max. process+thread count14780 (vs. ulimit of 30000)
modulesC: phoenix-core . U: .
Console outputhttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/4/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.

@stoty

stoty commented Sep 22, 2021

Copy link
Copy Markdown
ContributorAuthor

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 28sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 1sNo 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 _
+0 🆗mvndep5m 5sMaven dependency ordering for branch
+1 💚mvninstall16m 36smaster passed
+0hbaserecompile28m 5sHBase recompiled.
+1 💚compile1m 41smaster passed
+1 💚checkstyle5m 29smaster passed
+1 💚javadoc2m 6smaster passed
+0 🆗spotbugs4m 33sroot in master has 1031 extant spotbugs warnings.
+0 🆗spotbugs3m 3sphoenix-core in master has 972 extant spotbugs warnings.
_ Patch Compile Tests _
+0 🆗mvndep0m 16sMaven dependency ordering for patch
+1 💚mvninstall13m 37sthe patch passed
+0hbaserecompile20m 34sHBase recompiled.
+1 💚compile1m 40sthe patch passed
+1 💚javac1m 40sthe patch passed
-1 ❌checkstyle5m 31sroot: The patch generated 357 new + 9902 unchanged - 365 fixed = 10259 total (was 10267)
-1 ❌whitespace0m 0sThe patch 1 line(s) with tabs.
+1 💚xml0m 3sThe patch has no ill-formed XML file.
+1 💚javadoc2m 3sthe patch passed
+1 💚spotbugs3m 4sphoenix-core generated 0 new + 718 unchanged - 254 fixed = 718 total (was 972)
+1 💚spotbugs4m 30sroot generated 0 new + 771 unchanged - 260 fixed = 771 total (was 1031)
_ Other Tests _
-1 ❌unit115m 25sroot in the patch failed.
-1 ❌asflicense1m 53sThe patch generated 613 ASF License warnings.
204m 25s
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-1319/5/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#1319
Optional Testsdupname asflicense javac javadoc unit spotbugs hbaserebuild hbaseanti checkstyle compile xml
unameLinux e6080b08842c 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 / c1ffb0b
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-1319/5/artifact/yetus-general-check/output/diff-checkstyle-root.txt
whitespacehttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/5/artifact/yetus-general-check/output/whitespace-tabs.txt
unithttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/5/artifact/yetus-general-check/output/patch-unit-root.txt
Test Resultshttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/5/testReport/
asflicensehttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/5/artifact/yetus-general-check/output/patch-asflicense-problems.txt
Max. process+thread count13890 (vs. ulimit of 30000)
modulesC: phoenix-core . U: .
Console outputhttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1319/5/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.

richardantal
richardantal reviewed Sep 27, 2021
View reviewed changes

@richardantalrichardantal 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.

Looks good overall, but left some nits.
I see there is a separate ticket to handle the remaining SQL_NONCONSTANT_STRING_PASSED_TO_EXECUTE problems at https://issues.apache.org/jira/browse/PHOENIX-6560 .

Comment threadphoenix-core/src/main/java/org/apache/phoenix/expression/util/regex/JONIPattern.java Outdated
Comment threadphoenix-core/src/main/java/org/apache/phoenix/mapreduce/MultiHfileOutputFormat.java Outdated
Comment threadphoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexTool.java Outdated
Comment threadphoenix-core/src/main/java/org/apache/phoenix/parse/PFunction.java Outdated
Comment threadphoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java Outdated
Comment threadphoenix-core/src/main/java/org/apache/phoenix/query/ConnectionlessQueryServicesImpl.java
public MetaDataMutationResult getSchema(String schemaName, long clientTimestamp) throws SQLException {
try {
PSchema schema = metaData.getSchema(new PTableKey(null, schemaName));
new MetaDataMutationResult(MutationCode.SCHEMA_ALREADY_EXISTS, schema, 0);

@richardantalrichardantalSep 27, 2021

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.

I see that your change behaves the same way as it was before but, shouldn't we return this?

@stotystotySep 27, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No, this is a dummy class for certain tests.
See the javadoc comment on the class.

Comment threadphoenix-core/src/main/java/org/apache/phoenix/schema/stats/GuidePostsInfo.java Outdated
Comment threadphoenix-core/src/main/java/org/apache/phoenix/schema/tool/SchemaSynthesisProcessor.java Outdated
Comment threadphoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java Outdated
Comment threadphoenix-core/src/main/java/org/apache/phoenix/util/PhoenixRuntime.java Outdated
@stoty

stoty commented Sep 27, 2021

Copy link
Copy Markdown
ContributorAuthor

Thanks for the review @richardantal .
I have cleaned up what you suggested, and hopefully answered your question.

@richardantal
richardantal self-requested a review September 27, 2021 12:50
richardantal
richardantal approved these changes Sep 27, 2021
View reviewed changes

@richardantalrichardantal 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.

Thanks @stoty
LGTM

@stotystoty closed this Sep 29, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@virajjasanivirajjasanivirajjasani left review comments
@richardantalrichardantalrichardantal approved these changes
@gjacoby126gjacoby126Awaiting requested review from gjacoby126
@ChinmaySKulkarniChinmaySKulkarniAwaiting requested review from ChinmaySKulkarni
@apurtellapurtellAwaiting requested review from apurtell
@ankitsinghalankitsinghalAwaiting requested review from ankitsinghal
@joshelserjoshelserAwaiting requested review from joshelser

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@stoty@virajjasani@richardantal