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.

Sign up for GitHub

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

Jump to bottom

PHOENIX-7103 Phoenix-connectors uses the refactored ConnectionInfo class #111

Closed
wants to merge 2 commits into from
Closed

PHOENIX-7103 Phoenix-connectors uses the refactored ConnectionInfo class #111

wants to merge 2 commits into from

Conversation

stoty
Copy link
Contributor

@stoty stoty commented Nov 8, 2023

No description provided.

@stoty stoty requested a review from richardantal November 8, 2023 08:00
@stoty
Copy link
Contributor Author

stoty commented Nov 8, 2023

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 5m 16s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+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 _
+0 🆗 mvndep 10m 32s Maven dependency ordering for branch
+1 💚 mvninstall 14m 0s master passed
+1 💚 compile 1m 32s master passed
+1 💚 scaladoc 1m 11s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 9s Maven dependency ordering for patch
+1 💚 mvninstall 12m 35s the patch passed
+1 💚 compile 1m 31s the patch passed
+1 💚 scalac 1m 31s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 scaladoc 1m 13s the patch passed
_ Other Tests _
+1 💚 unit 28m 29s phoenix5-spark in the patch passed.
+1 💚 unit 1m 7s phoenix5-spark3 in the patch passed.
+1 💚 asflicense 0m 15s The patch does not generate ASF License warnings.
78m 17s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-Connectors-PreCommit-GitHub-PR/job/PR-111/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #111
Optional Tests dupname asflicense scalac scaladoc unit compile
uname Linux 80b211caaab8 4.15.0-213-generic #224-Ubuntu SMP Mon Jun 19 13:30:12 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev/phoenix-connectors-personality.sh
git revision master / 062b34c
Test Results https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-Connectors-PreCommit-GitHub-PR/job/PR-111/1/testReport/
Max. process+thread count 1928 (vs. ulimit of 30000)
modules C: phoenix5-spark phoenix5-spark3 U: .
Console output https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-Connectors-PreCommit-GitHub-PR/job/PR-111/1/console
versions git=2.7.4 maven=3.3.9
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

richardantal
richardantal reviewed Nov 8, 2023
View reviewed changes
phoenix5-spark/src/main/scala/org/apache/phoenix/spark/ConfigurationUtil.scala
if (parts.length >= 1)
conf.set(HConstants.ZOOKEEPER_QUORUM, parts(0).replaceAll("=", "\\\\:"))
if (parts.length >= 2)
conf.setInt(HConstants.ZOOKEEPER_CLIENT_PORT, Integer.parseInt(parts(1).replaceAll("=", "\\\\:")))
Copy link
Contributor

Choose a reason for hiding this comment

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

parts(1).replaceAll("=", "\\\\:")) should be a number, and I am not sure if it will be
if the zk Url is something like: jdbc:phoenix+zk:zk1-1\\:2181,zk1-2\\:2181::/hbase

escapedUrl will be like: zk1-1=2181,zk1-2=2181 : : /hbase
So the ports are in the part(0)
Is the zkUrl different than I think?

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, we should check if the string is empty.

@stoty stoty requested a review from richardantal November 8, 2023 12:57
@stoty
Copy link
Contributor Author

stoty commented Nov 8, 2023

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 18s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+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 _
+0 🆗 mvndep 10m 22s Maven dependency ordering for branch
+1 💚 mvninstall 13m 35s master passed
+1 💚 compile 1m 32s master passed
+1 💚 scaladoc 1m 12s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 10s Maven dependency ordering for patch
+1 💚 mvninstall 12m 43s the patch passed
+1 💚 compile 1m 31s the patch passed
+1 💚 scalac 1m 31s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 scaladoc 1m 12s the patch passed
_ Other Tests _
+1 💚 unit 28m 13s phoenix5-spark in the patch passed.
+1 💚 unit 1m 8s phoenix5-spark3 in the patch passed.
+1 💚 asflicense 0m 16s The patch does not generate ASF License warnings.
72m 41s
Subsystem Report/Notes
Docker ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-Connectors-PreCommit-GitHub-PR/job/PR-111/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #111
Optional Tests dupname asflicense scalac scaladoc unit compile
uname Linux 90c252c3f27f 4.15.0-213-generic #224-Ubuntu SMP Mon Jun 19 13:30:12 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev/phoenix-connectors-personality.sh
git revision master / 062b34c
Test Results https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-Connectors-PreCommit-GitHub-PR/job/PR-111/2/testReport/
Max. process+thread count 1922 (vs. ulimit of 30000)
modules C: phoenix5-spark phoenix5-spark3 U: .
Console output https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-Connectors-PreCommit-GitHub-PR/job/PR-111/2/console
versions git=2.7.4 maven=3.3.9
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

richardantal
richardantal approved these changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@richardantal richardantal left a comment

Choose a reason for hiding this comment

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

LGTM

@stoty stoty closed this Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardantal richardantal richardantal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stoty @richardantal