Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more tests for PHOENIX-6247 #1192

Closed
wants to merge 2 commits into from

Conversation

gokceni
Copy link
Contributor

@gokceni gokceni commented Apr 8, 2021

No description provided.

@stoty
Copy link
Contributor

stoty commented Apr 8, 2021

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 5m 36s 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-PHOENIX-6247 Compile Tests _
+1 💚 mvninstall 14m 51s 4.x-PHOENIX-6247 passed
+1 💚 compile 1m 5s 4.x-PHOENIX-6247 passed
+1 💚 checkstyle 1m 43s 4.x-PHOENIX-6247 passed
+1 💚 javadoc 0m 48s 4.x-PHOENIX-6247 passed
+0 🆗 spotbugs 3m 15s phoenix-core in 4.x-PHOENIX-6247 has 946 extant spotbugs warnings.
_ Patch Compile Tests _
+1 💚 mvninstall 6m 51s the patch passed
+1 💚 compile 1m 5s the patch passed
+1 💚 javac 1m 5s the patch passed
-1 ❌ checkstyle 1m 42s phoenix-core: The patch generated 20 new + 4691 unchanged - 3 fixed = 4711 total (was 4694)
-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 47s the patch passed
+1 💚 spotbugs 3m 31s the patch passed
_ Other Tests _
+1 💚 unit 196m 2s phoenix-core in the patch passed.
+1 💚 asflicense 0m 11s The patch does not generate ASF License warnings.
238m 25s
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-1192/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #1192
Optional Tests dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname Linux 8003fb7ca960 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev/phoenix-personality.sh
git revision 4.x-PHOENIX-6247 / 2279410
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-1192/1/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-1192/1/artifact/yetus-general-check/output/whitespace-eol.txt
Test Results https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1192/1/testReport/
Max. process+thread count 5105 (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-1192/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.

try (HBaseAdmin admin = conn.unwrap(PhoenixConnection.class).getQueryServices()
.getAdmin()) {

assertEquals(false, admin.tableExists(TableName.valueOf(fullTableHName)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't you already doing this check in test_bothTableAndIndexHaveDifferentNames which you just called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. Removing


// Drop row and check
conn.createStatement().execute("ALTER TABLE " + fullTableName + " DROP COLUMN NEW_COLUMN_1");

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we drop but don't check as the comment says we will? Or is the lack of an exception the only check we need here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update the comment. Yes lack of exception

@stoty
Copy link
Contributor

stoty commented Apr 9, 2021

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 2m 33s 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 1s 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-PHOENIX-6247 Compile Tests _
+1 💚 mvninstall 18m 40s 4.x-PHOENIX-6247 passed
+1 💚 compile 1m 17s 4.x-PHOENIX-6247 passed
+1 💚 checkstyle 1m 48s 4.x-PHOENIX-6247 passed
+1 💚 javadoc 0m 49s 4.x-PHOENIX-6247 passed
+0 🆗 spotbugs 3m 18s phoenix-core in 4.x-PHOENIX-6247 has 946 extant spotbugs warnings.
_ Patch Compile Tests _
+1 💚 mvninstall 6m 46s the patch passed
+1 💚 compile 1m 4s the patch passed
+1 💚 javac 1m 4s the patch passed
-1 ❌ checkstyle 1m 43s phoenix-core: The patch generated 17 new + 4694 unchanged - 0 fixed = 4711 total (was 4694)
-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 47s the patch passed
+1 💚 spotbugs 3m 29s the patch passed
_ Other Tests _
+1 💚 unit 198m 58s phoenix-core in the patch passed.
+1 💚 asflicense 0m 10s The patch does not generate ASF License warnings.
242m 21s
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-1192/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #1192
Optional Tests dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile
uname Linux 59c3c34c5b45 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev/phoenix-personality.sh
git revision 4.x-PHOENIX-6247 / 2279410
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-1192/2/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-1192/2/artifact/yetus-general-check/output/whitespace-eol.txt
Test Results https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1192/2/testReport/
Max. process+thread count 5142 (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-1192/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.

Copy link
Contributor

@gjacoby126 gjacoby126 left a comment

Choose a reason for hiding this comment

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

+1, thanks @gokceni

Copy link
Contributor

@swaroopak swaroopak left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -973,6 +976,25 @@ private PTable getTable(byte[] tenantId, byte[] schemaName, byte[] tableName, lo
return table;
}

private PName getPhysicalTableName(Region region, byte[] tenantId, byte[] schema, byte[] table, long timestamp) throws IOException {
Copy link
Contributor

@mnpoonia mnpoonia Apr 14, 2021

Choose a reason for hiding this comment

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

You think adding this to SchemaUtil make any sense. I see a lot of implementation of getPhysicalTableName there

@virajjasani
Copy link
Contributor

Thanks @gokceni. Is PR against master branch open?

@gokceni
Copy link
Contributor Author

gokceni commented Apr 19, 2021

@virajjasani I merged all of these on master and 4.x Closing

@gokceni gokceni closed this Apr 19, 2021
Sign up for free to 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.

6 participants