PHOENIX-5712 Got SYSCAT ILLEGAL_DATA exception after created tenant i… #972

yanxinyi · 2020-11-17T23:59:13Z

…ndex on view

stoty · 2020-11-18T12:56:06Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 18s	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.
		_ 4.x Compile Tests _
+1 💚	mvninstall	11m 44s	4.x passed
+1 💚	compile	1m 3s	4.x passed
+1 💚	checkstyle	1m 13s	4.x passed
+1 💚	javadoc	0m 48s	4.x passed
+0 🆗	spotbugs	3m 15s	phoenix-core in 4.x has 946 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	6m 28s	the patch passed
+1 💚	compile	1m 3s	the patch passed
+1 💚	javac	1m 3s	the patch passed
-1 ❌	checkstyle	1m 16s	phoenix-core: The patch generated 141 new + 2516 unchanged - 32 fixed = 2657 total (was 2548)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 46s	the patch passed
+1 💚	spotbugs	3m 31s	the patch passed
		_ Other Tests _
-1 ❌	unit	200m 27s	phoenix-core in the patch failed.
+1 💚	asflicense	0m 28s	The patch does not generate ASF License warnings.
		236m 1s

Reason	Tests
Failed junit tests	phoenix.end2end.ViewIndexIdRetrieveIT
	phoenix.end2end.NullIT
	phoenix.end2end.DropIndexedColsIT
	phoenix.end2end.BackwardCompatibilityIT

Subsystem	Report/Notes
Docker	ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-972/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#972
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname	Linux da6c1ab7e83a 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	4.x / `510ca96`
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-972/2/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-972/2/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-972/2/testReport/
Max. process+thread count	6074 (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-972/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.

phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java

…ndex on view

…-4.16 clients

yanxinyi · 2020-11-20T21:31:24Z

Added filter at the Coproc preScannerOpen method as we discussed offline. Please take a look again, thanks!

cc @jpisaac @ChinmaySKulkarni @kadirozde @lhofhansl

stoty · 2020-11-21T01:04:08Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 4s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 1s	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.
		_ 4.x Compile Tests _
+1 💚	mvninstall	12m 13s	4.x passed
+1 💚	compile	1m 0s	4.x passed
+1 💚	checkstyle	1m 13s	4.x passed
+1 💚	javadoc	0m 45s	4.x passed
+0 🆗	spotbugs	3m 11s	phoenix-core in 4.x has 950 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	6m 19s	the patch passed
+1 💚	compile	1m 1s	the patch passed
+1 💚	javac	1m 1s	the patch passed
-1 ❌	checkstyle	1m 13s	phoenix-core: The patch generated 80 new + 2509 unchanged - 3 fixed = 2589 total (was 2512)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 44s	the patch passed
+1 💚	spotbugs	3m 23s	the patch passed
		_ Other Tests _
-1 ❌	unit	181m 47s	phoenix-core in the patch failed.
+1 💚	asflicense	0m 29s	The patch does not generate ASF License warnings.
		216m 52s

Reason	Tests
Failed junit tests	phoenix.tx.TxCheckpointIT
	phoenix.end2end.index.SaltedIndexIT
	phoenix.end2end.AlterTableWithViewsIT
	phoenix.end2end.IndexScrutinyToolForTenantIT
	phoenix.end2end.PhoenixRuntimeIT

Subsystem	Report/Notes
Docker	ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-972/3/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#972
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname	Linux 7a72de256505 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	4.x / `e57fcc8`
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-972/3/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-972/3/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-972/3/testReport/
Max. process+thread count	5981 (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-972/3/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.

ChinmaySKulkarni · 2020-11-23T20:10:51Z

phoenix-core/src/main/java/org/apache/phoenix/filter/SyscatViewIndexIdFilter.java

+                // pre-splittable client should always using SMALLINT
+                if (type == NULL_DATA_TYPE_VALUE && viewIndexIdCell.getValueLength() >
+                        VIEW_INDEX_ID_SMALLINT_TYPE_VALUE_LEN) {
+                    Cell keyValue = ViewIndexIdRetrieveUtil.


What if there is a legitimate long number (not just extra bits, but actually large) here. We will shorten it to a SMALLINT and it becomes unusable right? Although based on offline discussions, seems like that case already means this view index ID is unqueryable. Should we trace log this or track it via a metric or something?

Can you provide an example of a legitimate long number? Do you mean when a value here is greater than the SHORT.MAX_VALUE? Yeah, if that's the case the value that we send back to the client will lose some data precision but still queryable.

phoenix-core/src/main/java/org/apache/phoenix/filter/SyscatViewIndexIdFilter.java

ChinmaySKulkarni · 2020-11-25T00:56:15Z

Had a quick glance and added some review comments. As discussed offline, please add more comments based on various if/else conditions for when we do the conversion vs. when we don't/can't.

phoenix-core/src/it/java/org/apache/phoenix/end2end/ViewIndexIdRetrieveIT.java

phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java

ChinmaySKulkarni · 2020-11-25T01:12:13Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/SyscatRegionObserver.java

+/**
+ * Coprocessor that checks whether the VIEW_INDEX_ID needs to retrieve.
+ */
+public class SyscatRegionObserver extends BaseRegionObserver {


Can we rename this coproc so it indicates that it is for view_index_id queries on system.catalog instead of this generic name?

if we want to do something special for the syscat in the future, we don't need to create a new coproc. In fact, we can add on new logic to this existing coproc.

Ok makes sense. Can you rename it to SystemCatalogRegionObserver in that case?

sure. renamed to SystemCatalogRegionObserver

ChinmaySKulkarni · 2020-11-25T01:24:11Z

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/SyscatRegionObserver.java

+    public RegionScanner preScannerOpen(ObserverContext<RegionCoprocessorEnvironment> e, Scan scan,
+                                        RegionScanner s) throws IOException {
+        int clientVersion = ScanUtil.getClientVersion(scan);
+        if (clientVersion != UNKNOWN_CLIENT_VERSION) {


nit: Not sure I understand why we don't care about 4.4.0 clients (not that it matters really). What is it for?

since corpoc is not only for the phoenix, but it also works for the HBase. We don't want to change the behavior other than the Phoenix environment.
The ScanUtil.getClientVersion returns UNKNOWN_CLIENT_VERSION if the phoenix client version is not setup.

phoenix-core/src/main/java/org/apache/phoenix/filter/SyscatViewIndexIdFilter.java

…o long

stoty · 2020-11-25T08:44:53Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 43s	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.
		_ 4.x Compile Tests _
+1 💚	mvninstall	12m 22s	4.x passed
+1 💚	compile	1m 1s	4.x passed
+1 💚	checkstyle	1m 7s	4.x passed
+1 💚	javadoc	0m 49s	4.x passed
+0 🆗	spotbugs	3m 39s	phoenix-core in 4.x has 950 extant spotbugs warnings.
-0 ⚠️	patch	3m 47s	Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
		_ Patch Compile Tests _
+1 💚	mvninstall	6m 51s	the patch passed
+1 💚	compile	1m 1s	the patch passed
+1 💚	javac	1m 1s	the patch passed
-1 ❌	checkstyle	1m 11s	phoenix-core: The patch generated 82 new + 2512 unchanged - 1 fixed = 2594 total (was 2513)
+1 💚	whitespace	0m 1s	The patch has no whitespace issues.
+1 💚	javadoc	0m 48s	the patch passed
+1 💚	spotbugs	3m 33s	the patch passed
		_ Other Tests _
-1 ❌	unit	216m 14s	phoenix-core in the patch failed.
+1 💚	asflicense	0m 49s	The patch does not generate ASF License warnings.
		254m 13s

Reason	Tests
Failed junit tests	phoenix.end2end.IndexToolIT
	phoenix.end2end.index.LocalIndexIT
	phoenix.end2end.IndexToolForNonTxGlobalIndexIT

Subsystem	Report/Notes
Docker	ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-972/4/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#972
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname	Linux 6a158be4faaf 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	4.x / `c3818ee`
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-972/4/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-972/4/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-972/4/testReport/
Max. process+thread count	6341 (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-972/4/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.

ChinmaySKulkarni

Couple of nits otherwise +1

ChinmaySKulkarni · 2020-11-25T21:41:08Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/ViewIndexIdRetrieveIT.java

+    a coproc that checks the client request version and send it back to the client.
+    For more information, please see PHOENIX-3547, PHOENIX-5712
+ */
+public class ViewIndexIdRetrieveIT extends BaseOwnClusterIT {


The comment in BaseOwnClusterIT says "Any new integration tests that need their own mini cluster should be extending {@link BaseUniqueNamesOwnClusterIT} class directly" so maybe we want to do that and then use uniqueNames for all table/view/index names etc.

changed to BaseUniqueNamesOwnClusterIT.
All table/view/index are all using generateUniqueName method to get a unique name.

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/SyscatRegionObserver.java

ChinmaySKulkarni · 2020-11-25T23:48:13Z

phoenix-core/src/main/java/org/apache/phoenix/filter/SystemCatalogViewIndexIdFilter.java

+            This is combination of diff client created view index looks like:
+
+            client                  VIEW_INDEX_ID(Cell number of bytes)     VIEW_INDEX_ID_DATA_TYPE
+        pre-4.14                        2 bytes                                     NULL


this should be pre-4.15 right?

right. fixed typo

phoenix-core/src/main/java/org/apache/phoenix/filter/SystemCatalogViewIndexIdFilter.java

stoty · 2020-11-26T05:19:33Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 11s	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.
		_ 4.x Compile Tests _
+1 💚	mvninstall	11m 0s	4.x passed
+1 💚	compile	0m 55s	4.x passed
+1 💚	checkstyle	1m 24s	4.x passed
+1 💚	javadoc	0m 44s	4.x passed
+0 🆗	spotbugs	3m 17s	phoenix-core in 4.x has 950 extant spotbugs warnings.
-0 ⚠️	patch	3m 24s	Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary.
		_ Patch Compile Tests _
+1 💚	mvninstall	5m 34s	the patch passed
+1 💚	compile	0m 56s	the patch passed
+1 💚	javac	0m 56s	the patch passed
-1 ❌	checkstyle	1m 38s	phoenix-core: The patch generated 83 new + 2517 unchanged - 1 fixed = 2600 total (was 2518)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	0m 49s	the patch passed
+1 💚	spotbugs	3m 11s	the patch passed
		_ Other Tests _
-1 ❌	unit	214m 53s	phoenix-core in the patch failed.
+1 💚	asflicense	0m 35s	The patch does not generate ASF License warnings.
		248m 53s

Reason	Tests
Failed junit tests	phoenix.end2end.InQueryIT
	phoenix.end2end.IndexExtendedIT
	phoenix.end2end.AlterTableWithViewsIT
	phoenix.end2end.AggregateQueryIT
	phoenix.end2end.index.IndexMetadataIT
	phoenix.end2end.ConcurrentMutationsExtendedIT

Subsystem	Report/Notes
Docker	ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-972/5/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#972
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname	Linux 14cd02cf6827 4.15.0-65-generic #74-Ubuntu SMP Tue Sep 17 17:06:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev/phoenix-personality.sh
git revision	4.x / `7ac4dff`
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-972/5/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-972/5/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-972/5/testReport/
Max. process+thread count	6230 (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-972/5/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.

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/SystemCatalogRegionObserver.java

yanxinyi force-pushed the PHOENIX-5712-COPROC branch 2 times, most recently from 6a5a89e to 5ff9c76 Compare November 18, 2020 07:16

jpisaac reviewed Nov 20, 2020

View reviewed changes

phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java Outdated Show resolved Hide resolved

yanxinyi added 4 commits November 20, 2020 13:05

PHOENIX-5712 Got SYSCAT ILLEGAL_DATA exception after created tenant i…

4fb93cb

…ndex on view

using CellUtil to get viewIndexId and DataType Cells

67f3ad9

implemented HBaseFilter for 4.16 client and added ignore flag for pre…

620df93

…-4.16 clients

moved filter logic to the server side

6b5bb0e

yanxinyi force-pushed the PHOENIX-5712-COPROC branch from 3c45566 to 6b5bb0e Compare November 20, 2020 21:26

ChinmaySKulkarni reviewed Nov 25, 2020

View reviewed changes

renamed, fix nits, used api to retrieve the long to short and short t…

f66f607

…o long

ChinmaySKulkarni approved these changes Nov 25, 2020

View reviewed changes

renamed and fixed nits

e8292f7

kadirozde reviewed Dec 1, 2020

View reviewed changes

phoenix-core/src/main/java/org/apache/phoenix/coprocessor/SystemCatalogRegionObserver.java Show resolved Hide resolved

yanxinyi closed this Dec 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHOENIX-5712 Got SYSCAT ILLEGAL_DATA exception after created tenant i… #972

PHOENIX-5712 Got SYSCAT ILLEGAL_DATA exception after created tenant i… #972

yanxinyi commented Nov 17, 2020

stoty commented Nov 18, 2020

yanxinyi commented Nov 20, 2020 •

edited

Loading

stoty commented Nov 21, 2020

ChinmaySKulkarni Nov 23, 2020

yanxinyi Nov 25, 2020

ChinmaySKulkarni commented Nov 25, 2020 •

edited

Loading

ChinmaySKulkarni Nov 25, 2020

yanxinyi Nov 25, 2020 •

edited

Loading

ChinmaySKulkarni Nov 25, 2020

yanxinyi Nov 26, 2020

ChinmaySKulkarni Nov 25, 2020

yanxinyi Nov 25, 2020

stoty commented Nov 25, 2020

ChinmaySKulkarni left a comment

ChinmaySKulkarni Nov 25, 2020

yanxinyi Nov 26, 2020

ChinmaySKulkarni Nov 25, 2020

yanxinyi Nov 26, 2020

stoty commented Nov 26, 2020

PHOENIX-5712 Got SYSCAT ILLEGAL_DATA exception after created tenant i… #972

PHOENIX-5712 Got SYSCAT ILLEGAL_DATA exception after created tenant i… #972

Conversation

yanxinyi commented Nov 17, 2020

stoty commented Nov 18, 2020

yanxinyi commented Nov 20, 2020 • edited Loading

stoty commented Nov 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChinmaySKulkarni commented Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

yanxinyi Nov 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoty commented Nov 25, 2020

ChinmaySKulkarni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoty commented Nov 26, 2020

yanxinyi commented Nov 20, 2020 •

edited

Loading

ChinmaySKulkarni commented Nov 25, 2020 •

edited

Loading

yanxinyi Nov 25, 2020 •

edited

Loading