PHOENIX-6386 Bulkload generates unverified index rows by stoty · Pull Request #1144 · apache/phoenix

stoty · 2021-02-18T10:40:26Z

No description provided.

stoty · 2021-02-18T17:02:19Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 16s	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	15m 28s	master passed
+0	hbaserecompile	22m 44s	HBase recompiled.
+1 💚	compile	1m 6s	master passed
+1 💚	checkstyle	0m 35s	master passed
+1 💚	javadoc	0m 52s	master passed
+0 🆗	spotbugs	3m 6s	phoenix-core in master has 959 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	8m 31s	the patch passed
+0	hbaserecompile	18m 2s	HBase recompiled.
+1 💚	compile	1m 3s	the patch passed
+1 💚	javac	1m 3s	the patch passed
-1 ❌	checkstyle	0m 36s	phoenix-core: The patch generated 33 new + 138 unchanged - 4 fixed = 171 total (was 142)
-1 ❌	whitespace	0m 0s	The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
+1 💚	javadoc	0m 51s	the patch passed
+1 💚	spotbugs	3m 17s	the patch passed
		_ Other Tests _
+1 💚	unit	237m 17s	phoenix-core in the patch passed.
+1 💚	asflicense	0m 17s	The patch does not generate ASF License warnings.
		289m 19s

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-1144/3/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1144
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaserebuild hbaseanti checkstyle compile
uname	Linux 4500104df372 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	master / `34113c5`
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-1144/3/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt
whitespace	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1144/3/artifact/yetus-general-check/output/whitespace-eol.txt
Test Results	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1144/3/testReport/
Max. process+thread count	10175 (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-1144/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.

virajjasani

Left few comments, looks good overall

virajjasani · 2021-02-21T14:09:06Z

 for (Cell kv : keyValueList) {
-list.add(kv);
+cellsForTable.add(kv);
 }


nit: can be replaced with cellsForTable.addAll(keyValueList)

virajjasani · 2021-02-21T14:10:45Z

+ for(int i=0; i < keyValues.size() ; i++) {
+ Cell kv = keyValues.get(i);


can be replaced by
for (Cell kv : keyValues) {

I purposefully chose this construct, as we're processing a lot a objects, and this is reportedly faster and easier on memory / GC.

virajjasani · 2021-02-21T14:11:32Z

+ * 
+ * @param keyValues will be modified
+ */
+ public void setVerfied(List<Cell> keyValues) {


nit: "s/setVerfied/setVerified"

virajjasani · 2021-02-21T14:13:06Z

+ private byte[] emptyKeyValueCF;
+ private int emptyKeyValueCFLength;
+ private byte[] emptyKeyValueQualifier;
+ private int emptyKeyValueQualifierLength;


nit: good to mark all final

virajjasani · 2021-02-21T14:16:14Z

+ checkIndexTableIsVerfied("TABLE3_IDX");
+ }
+
+ private void checkIndexTableIsVerfied(String indexTableName) throws SQLException, IOException {


nit: "s/checkIndexTableIsVerfied/checkIndexTableIsVerified"

virajjasani · 2021-02-21T14:18:58Z

+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.Result;


Few unused imports, nothing urgent, can be removed while committing changes

virajjasani · 2021-02-21T14:24:17Z

+ if(indexStatusUpdaters[i] != null) {
+ indexStatusUpdaters[i].setVerfied(keyValueList);
+ }


This should solve the problem of verifying index rows right?

Strictly speaking, we are not verifying them, as we're generating the records in bulk, and we assume that they are correct (we assume that the bluk load job runs fully).
We're just setting the status to verified to avoid repairing the freshly generated index rows.

Got it, makes sense

stoty

Thanks for the thorough review @virajjasani

stoty · 2021-02-22T05:28:23Z

+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.Result;


stoty · 2021-02-22T05:28:36Z

+ checkIndexTableIsVerfied("TABLE3_IDX");
+ }
+
+ private void checkIndexTableIsVerfied(String indexTableName) throws SQLException, IOException {


stoty · 2021-02-22T05:30:54Z

+ if(indexStatusUpdaters[i] != null) {
+ indexStatusUpdaters[i].setVerfied(keyValueList);
+ }


Strictly speaking, we are not verifying them, as we're generating the records in bulk, and we assume that they are correct (we assume that the bluk load job runs fully).
We're just setting the status to verified to avoid repairing the freshly generated index rows.

stoty · 2021-02-22T05:31:04Z

+ private byte[] emptyKeyValueCF;
+ private int emptyKeyValueCFLength;
+ private byte[] emptyKeyValueQualifier;
+ private int emptyKeyValueQualifierLength;


stoty · 2021-02-22T05:31:34Z

+ * 
+ * @param keyValues will be modified
+ */
+ public void setVerfied(List<Cell> keyValues) {


stoty · 2021-02-22T05:32:59Z

+ for(int i=0; i < keyValues.size() ; i++) {
+ Cell kv = keyValues.get(i);


I purposefully chose this construct, as we're processing a lot a objects, and this is reportedly faster and easier on memory / GC.

stoty · 2021-02-22T05:35:21Z

 for (Cell kv : keyValueList) {
-list.add(kv);
+cellsForTable.add(kv);
 }


stoty · 2021-02-22T08:37:30Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	1m 10s	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	15m 31s	master passed
+0	hbaserecompile	22m 5s	HBase recompiled.
+1 💚	compile	1m 7s	master passed
+1 💚	checkstyle	0m 36s	master passed
+1 💚	javadoc	0m 53s	master passed
+0 🆗	spotbugs	3m 6s	phoenix-core in master has 959 extant spotbugs warnings.
		_ Patch Compile Tests _
+1 💚	mvninstall	8m 14s	the patch passed
+0	hbaserecompile	18m 13s	HBase recompiled.
+1 💚	compile	1m 3s	the patch passed
+1 💚	javac	1m 3s	the patch passed
-1 ❌	checkstyle	0m 37s	phoenix-core: The patch generated 24 new + 138 unchanged - 4 fixed = 162 total (was 142)
-1 ❌	whitespace	0m 0s	The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
+1 💚	javadoc	0m 50s	the patch passed
+1 💚	spotbugs	3m 20s	the patch passed
		_ Other Tests _
+1 💚	unit	107m 14s	phoenix-core in the patch passed.
+1 💚	asflicense	0m 16s	The patch does not generate ASF License warnings.
		158m 44s

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-1144/4/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#1144
Optional Tests	dupname asflicense javac javadoc unit spotbugs hbaserebuild hbaseanti checkstyle compile
uname	Linux ea68963707d2 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	master / `34113c5`
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-1144/4/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt
whitespace	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1144/4/artifact/yetus-general-check/output/whitespace-eol.txt
Test Results	https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1144/4/testReport/
Max. process+thread count	10892 (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-1144/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.

stoty · 2021-02-22T08:40:47Z

@virajjasani If the changes look good can you add a formal +1 / approved ?

virajjasani

+1

stoty force-pushed the PHOENIX-6386 branch from 0cf1795 to f9482afCompare February 18, 2021 10:41

PHOENIX-6386 Bulkload generates unverified index rows
42607b0

stoty force-pushed the PHOENIX-6386 branch from e9aae9e to 42607b0Compare February 18, 2021 12:12

stoty requested review from kadirozde and virajjasani February 18, 2021 12:27

virajjasani reviewed Feb 21, 2021
View reviewed changes

stoty commented Feb 22, 2021
View reviewed changes

address review comments, and some checkstyle issues
659279e

virajjasani approved these changes Feb 22, 2021
View reviewed changes

stoty closed this Feb 22, 2021

stoty deleted the PHOENIX-6386 branch July 29, 2021 14:37

		for(int i=0; i < keyValues.size() ; i++) {
		Cell kv = keyValues.get(i);

		import org.apache.hadoop.hbase.client.Admin;
		import org.apache.hadoop.hbase.client.Result;

Uh oh!

Conversation

stoty commented Feb 18, 2021

Uh oh!

stoty commented Feb 18, 2021

Uh oh!

virajjasani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virajjasaniFeb 21, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoty commented Feb 22, 2021

Uh oh!

stoty commented Feb 22, 2021

Uh oh!

virajjasani left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

virajjasaniFeb 21, 2021•
edited
Loading