Skip to content

PHOENIX-6447: Add support for SYSTEM.CHILD_LINK table in systemcatalogwalentryfilter - #1201

Merged
gjacoby126 merged 1 commit into
apache:4.16from
sandeepvinayak:PHOENIX-6447-4.16
Apr 22, 2021
Merged

PHOENIX-6447: Add support for SYSTEM.CHILD_LINK table in systemcatalogwalentryfilter#1201
gjacoby126 merged 1 commit into
apache:4.16from
sandeepvinayak:PHOENIX-6447-4.16

Conversation

@sandeepvinayak

Copy link
Copy Markdown
Contributor

No description provided.

@sandeepvinayak

Copy link
Copy Markdown
ContributorAuthor

@gjacoby126@yanxinyi can you please review this to add system child link table in systemcatalog wal entry filter for replication.

@stoty

Copy link
Copy Markdown
Contributor

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec5m 36sDocker 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.
_ 4.16 Compile Tests _
+1 💚mvninstall13m 40s4.16 passed
+1 💚compile1m 0s4.16 passed
+1 💚checkstyle0m 27s4.16 passed
+1 💚javadoc0m 45s4.16 passed
+0 🆗spotbugs2m 59sphoenix-core in 4.16 has 944 extant spotbugs warnings.
_ Patch Compile Tests _
+1 💚mvninstall5m 41sthe patch passed
+1 💚compile1m 2sthe patch passed
+1 💚javac1m 2sthe patch passed
-1 ❌checkstyle0m 27sphoenix-core: The patch generated 19 new + 4 unchanged - 7 fixed = 23 total (was 11)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚javadoc0m 44sthe patch passed
+1 💚spotbugs3m 11sthe patch passed
_ Other Tests _
+1 💚unit127m 36sphoenix-core in the patch passed.
+1 💚asflicense0m 11sThe patch does not generate ASF License warnings.
164m 17s
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1201/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#1201
JIRA IssuePHOENIX-6447
Optional Testsdupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
unameLinux b45fec623579 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 revision4.16 / 0a79cb6
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-1201/1/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt
Test Resultshttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1201/1/testReport/
Max. process+thread count5623 (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-1201/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.

@sandeepvinayak
sandeepvinayakforce-pushed the PHOENIX-6447-4.16 branch 2 times, most recently from 2ecde33 to fc9792fCompareApril 21, 2021 18:06
private boolean isTenantRowCellSystemChildLink(final Cell cell) {
ImmutableBytesWritable key = new ImmutableBytesWritable(
cell.getRowArray(), cell.getRowOffset(), cell.getRowLength());
//rows in system.catalog that aren't tenant-owned will

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.

nit: Remove system.catalog from comments.

cell.getRowArray(), cell.getRowOffset(), cell.getRowLength());
//rows in system.catalog that aren't tenant-owned will
// have a leading separator byte
boolean isTenantRowCell =

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

Does the schema for system.child_link table the same as that for system.catalog? and does this assumption of leading separator byte holds true there as well? If so we can refactor isTenantRowCellSystemCatalog and reuse the same function here.

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.

The schema remains the same but the system catalog table is not supposed to have tenant-owned data with empty tenant id anymore. Thus, I believe we need different methods.

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

Looks good, just a couple nits

@stoty

Copy link
Copy Markdown
Contributor

💔 -1 overall

VoteSubsystemRuntimeComment
+0 🆗reexec0m 31sDocker 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.
_ 4.16 Compile Tests _
+1 💚mvninstall13m 54s4.16 passed
+1 💚compile1m 0s4.16 passed
+1 💚checkstyle0m 26s4.16 passed
+1 💚javadoc0m 44s4.16 passed
+0 🆗spotbugs2m 59sphoenix-core in 4.16 has 944 extant spotbugs warnings.
_ Patch Compile Tests _
+1 💚mvninstall5m 41sthe patch passed
+1 💚compile0m 58sthe patch passed
+1 💚javac0m 58sthe patch passed
-1 ❌checkstyle0m 27sphoenix-core: The patch generated 4 new + 4 unchanged - 7 fixed = 8 total (was 11)
+1 💚whitespace0m 0sThe patch has no whitespace issues.
+1 💚javadoc0m 45sthe patch passed
-1 ❌spotbugs3m 12sphoenix-core generated 1 new + 944 unchanged - 0 fixed = 945 total (was 944)
_ Other Tests _
+1 💚unit127m 50sphoenix-core in the patch passed.
+1 💚asflicense0m 11sThe patch does not generate ASF License warnings.
159m 30s
ReasonTests
FindBugsmodule:phoenix-core
Unread field:field be static? At SystemCatalogWALEntryFilter.java:[line 49]
SubsystemReport/Notes
DockerClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1201/4/artifact/yetus-general-check/output/Dockerfile
GITHUB PR#1201
JIRA IssuePHOENIX-6447
Optional Testsdupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
unameLinux 1c2d68d8a757 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitydev/phoenix-personality.sh
git revision4.16 / 0a79cb6
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-1201/4/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt
spotbugshttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1201/4/artifact/yetus-general-check/output/new-spotbugs-phoenix-core.html
Test Resultshttps://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1201/4/testReport/
Max. process+thread count5643 (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-1201/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.

@gjacoby126

Copy link
Copy Markdown
Contributor

@sandeepvinayak - could you please change your commit message to match the JIRA name (with the JIRA number). Then this looks good to go.

@sandeepvinayak

Copy link
Copy Markdown
ContributorAuthor

@gjacoby126 changed the commit message

@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 for the patch @sandeepvinayak

@gjacoby126
gjacoby126 merged commit 3bf2e95 into apache:4.16Apr 22, 2021
gjacoby126 pushed a commit that referenced this pull request Apr 22, 2021
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.

4 participants

@sandeepvinayak@stoty@gjacoby126@ankitjain64