PHOENIX-6506 : Tenant Connection is not able to access/validate Global Sequences by lokiore · Pull Request #1261 · apache/phoenix

lokiore · 2021-07-07T08:45:23Z

No description provided.

stoty · 2021-07-07T11:48:06Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 31s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s	The 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 💚	mvninstall	23m 55s	master passed
+0	hbaserecompile	30m 55s	HBase recompiled.
+1 💚	compile	1m 4s	master passed
+1 💚	checkstyle	1m 8s	master passed
+1 💚	javadoc	0m 53s	master passed
+0 🆗	spotbugs	3m 6s	phoenix-core in master has 965 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	13m 39s	the patch passed
+0	hbaserecompile	23m 44s	HBase recompiled.
+1 💚	compile	1m 4s	the patch passed
+1 💚	javac	1m 4s	the patch passed
-1 ❌	checkstyle	1m 29s	phoenix-core: The patch generated 14 new + 2227 unchanged - 0 fixed = 2241 total (was 2227)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 51s	the patch passed
+1 💚	spotbugs	3m 16s	the patch passed
		_ Other Tests _
-1 ❌	unit	113m 14s	phoenix-core in the patch failed.
+1 💚	asflicense	0m 43s	The patch does not generate ASF License warnings.
		182m 6s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1261/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1261
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaserebuild hbaseanti checkstyle compile
uname	Linux 416eee8a37f0 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 tool	maven
Personality	dev/phoenix-personality.sh
git revision	master / `62f63c8`
Default Java	Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08
checkstyle	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1261/1/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt
unit	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1261/1/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt
Test Results	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1261/1/testReport/
Max. process+thread count	15650 (vs. ulimit of 30000)
modules	C: phoenix-core U: phoenix-core
Console output	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1261/1/console
versions	git=2.7.4 maven=3.3.9 spotbugs=4.1.3
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

mnpoonia · 2021-07-15T13:14:19Z

@lokiore - Looks like there are checkstyle issues and some unit test are failing

virajjasani

Skimmed, left few comments

virajjasani · 2021-07-16T14:36:41Z

@abhishek-chouhan @dbwong @gjacoby126 Could you please take a look when you have some bandwidth?

virajjasani

Left few more comments

virajjasani · 2021-07-19T16:37:12Z

@stoty I agree and we have 4.x backport PR #1270 with proper Java 7 semantics so I think we are good with this PR using streams and eventually land on master and 5.1.
4.x branch is still stuck with Java 7 because of HBase 1 support but our CI builds still use JDK 1.8 so it doesn't catch such issues.

virajjasani

+1, pending QA

abhishek-chouhan · 2021-07-19T18:09:52Z

I'm not sure if this is a bug. Tenants are allowed to create their own sequences and use those for their operations. In general tenant connections are not able to see data associated with a global non-tenant connection. This could also be considered similar to that. I do think there is value in having a shared sequence across tenants, however need to think more if that should be through enabling it generally, like this pr aims to do, or having something like a "multi-tenant" sequence (create sequnce .. multi-tenant=true). @gjacoby126 @virajjasani

stoty · 2021-07-19T18:36:46Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 31s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	hbaseanti	0m 0s	Patch does not have any anti-patterns.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s	The 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 💚	mvninstall	22m 24s	master passed
+0	hbaserecompile	29m 16s	HBase recompiled.
+1 💚	compile	1m 3s	master passed
+1 💚	checkstyle	1m 26s	master passed
+1 💚	javadoc	0m 53s	master passed
+0 🆗	spotbugs	3m 22s	phoenix-core in master has 964 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	14m 23s	the patch passed
+0	hbaserecompile	24m 34s	HBase recompiled.
+1 💚	compile	1m 7s	the patch passed
+1 💚	javac	1m 7s	the patch passed
-1 ❌	checkstyle	1m 24s	phoenix-core: The patch generated 20 new + 2221 unchanged - 6 fixed = 2241 total (was 2227)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 52s	the patch passed
+1 💚	spotbugs	3m 32s	the patch passed
		_ Other Tests _
+1 💚	unit	113m 34s	phoenix-core in the patch passed.
+1 💚	asflicense	0m 18s	The patch does not generate ASF License warnings.
		179m 51s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1261/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1261
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaserebuild hbaseanti checkstyle compile
uname	Linux 69278de4d602 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 tool	maven
Personality	dev/phoenix-personality.sh
git revision	master / `6842c1d`
Default Java	Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08
checkstyle	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1261/2/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt
Test Results	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1261/2/testReport/
Max. process+thread count	14213 (vs. ulimit of 30000)
modules	C: phoenix-core U: phoenix-core
Console output	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1261/2/console
versions	git=2.7.4 maven=3.3.9 spotbugs=4.1.3
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

virajjasani · 2021-07-20T11:37:58Z

I do think there is value in having a shared sequence across tenants, however need to think more if that should be through enabling it generally, like this pr aims to do, or having something like a "multi-tenant" sequence (create sequnce .. multi-tenant=true)

Nice one, yeah if not the global sequence, maybe a special type of multi-tenant sequence (i.e new attribute attached to sequence) that can make it accessible to all tenants.

abhishek-chouhan · 2021-07-20T15:56:59Z

Lets repurpose/open a new jira, which introduces multi-tenant sequences unless there are objections.

gjacoby126 · 2021-07-21T12:21:26Z

@abhishek-chouhan @virajjasani - I believe the usual pattern in Phoenix is that a tenant connection should already be able to see global things. For example, if you get a tenant connection and query a non-MT table, you should be able to get rows back. Tenant connections afaik just add "WHERE [TenantId Column] = [Your connection's tenantId]" to all multi-tenant tables.

lokiore · 2021-07-24T14:52:49Z

@gjacoby126 @abhishek-chouhan @virajjasani - What you guys think should be next steps for this!?
Is this good or we need to repurpose!?

virajjasani · 2021-07-27T14:41:44Z

I believe the usual pattern in Phoenix is that a tenant connection should already be able to see global things. For example, if you get a tenant connection and query a non-MT table, you should be able to get rows back.

Tenant connection can read from global views as well. I think this approach also would be good enough i.e having global sequence accessible to tenant connection. If we are good to go with this approach, changes look good to me per my previous reviews. I was just curious about the final decision on the approach we decide to go ahead with.
@abhishek-chouhan WDYT? Does this sound good to you?

abhishek-chouhan · 2021-07-27T16:02:57Z

Makes sense. Should be fine to go ahead with this since it gives us parity with the current behavior as @gjacoby126 rightly pointed out.

virajjasani · 2021-07-28T06:51:09Z

Sounds good. Thanks @abhishek-chouhan @gjacoby126 for your valuable inputs.
Will merge this after 24 hr unless there are any objections on this PR.

…l Sequences (#1261) Signed-off-by: Viraj Jasani <vjasani@apache.org>

…l Sequences (#1270) (#1261) Signed-off-by: Viraj Jasani <vjasani@apache.org>

Allowing Tenant Connections to Access Global Sequences
24cbc49

virajjasani requested a review from abhishek-chouhan July 16, 2021 14:12

virajjasani reviewed Jul 16, 2021
View reviewed changes

Comment threadphoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java Outdated
Comment threadphoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java Outdated

virajjasani requested a review from gjacoby126 July 16, 2021 15:35

virajjasani reviewed Jul 16, 2021
View reviewed changes

Comment threadphoenix-core/src/it/java/org/apache/phoenix/end2end/UpsertValuesIT.java
Comment threadphoenix-core/src/it/java/org/apache/phoenix/end2end/UpsertValuesIT.java

Using lamda and simplifying comparision
21a1ebf

virajjasani approved these changes Jul 19, 2021
View reviewed changes

virajjasani merged commit a4f0304 into apache:masterJul 29, 2021

virajjasani pushed a commit that referenced this pull request Jul 29, 2021
PHOENIX-6506: Tenant Connection is not able to access/validate Globa…
6c2a802
…l Sequences (#1261) Signed-off-by: Viraj Jasani <vjasani@apache.org>

virajjasani pushed a commit that referenced this pull request Jul 29, 2021
PHOENIX-6506: Tenant Connection is not able to access/validate Globa…
2bdd4a2
…l Sequences (#1270) (#1261) Signed-off-by: Viraj Jasani <vjasani@apache.org>

virajjasani pushed a commit that referenced this pull request Jul 29, 2021
PHOENIX-6506: Tenant Connection is not able to access/validate Globa…
2605cc2
…l Sequences (#1270) (#1261) Signed-off-by: Viraj Jasani <vjasani@apache.org>

Uh oh!

Conversation

lokiore commented Jul 7, 2021

Uh oh!

stoty commented Jul 7, 2021

Uh oh!

mnpoonia commented Jul 15, 2021

Uh oh!

virajjasani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

virajjasani commented Jul 16, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

virajjasani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

virajjasani commented Jul 19, 2021

Uh oh!

virajjasani left a comment

Choose a reason for hiding this comment

Uh oh!

abhishek-chouhan commented Jul 19, 2021

Uh oh!

stoty commented Jul 19, 2021

Uh oh!

virajjasani commented Jul 20, 2021

Uh oh!

abhishek-chouhan commented Jul 20, 2021

Uh oh!

gjacoby126 commented Jul 21, 2021

Uh oh!

lokiore commented Jul 24, 2021

Uh oh!

virajjasani commented Jul 27, 2021

Uh oh!

abhishek-chouhan commented Jul 27, 2021

Uh oh!

virajjasani commented Jul 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

virajjasani commented Jul 16, 2021•
edited
Loading