Skip to content

HBase Connection Registry usage with Phoenix JDBC url#24869

Merged
mosabua merged 1 commit intotrinodb:masterfrom
virajjasani:reg
Feb 12, 2025
Merged

HBase Connection Registry usage with Phoenix JDBC url#24869
mosabua merged 1 commit intotrinodb:masterfrom
virajjasani:reg

Conversation

@virajjasani
Copy link
Copy Markdown
Member

@virajjasani virajjasani commented Jan 31, 2025

Description

Use various HBase Connection Registries with Phoenix JDBC url formats. Document with recommendations and use in test.

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

@virajjasani
Copy link
Copy Markdown
Member Author

virajjasani commented Jan 31, 2025

I have run TestPhoenixConnectorTest locally 3 times and all 241 out of 337 (96 ignored) tests are passing each time. Not sure why the registry based exception would occur (unless the master daemon is not stable or does not have meta cache).

Edit: updated test to use new format for ZK ConnectionRegistry. RPC Registry for test is facing errors because in docker env, clients can exhaust master with RPC threadpool while it is already running in-house chores.

@virajjasani virajjasani force-pushed the reg branch 5 times, most recently from 2cfd35e to 9f24500 Compare February 1, 2025 03:02
@virajjasani virajjasani requested review from ebyhr and mosabua February 4, 2025 04:50
@ebyhr ebyhr removed their request for review February 4, 2025 04:54
@virajjasani virajjasani requested a review from wendigo February 4, 2025 16:40
@lhofhansl
Copy link
Copy Markdown
Member

Looks good. +1

@virajjasani
Copy link
Copy Markdown
Member Author

Thanks @lhofhansl!

FYI @stoty if you would also like to take a look.

@virajjasani
Copy link
Copy Markdown
Member Author

@wendigo @mosabua I am curious reg the guidelines, as long as there is no merge conflict, is it fine to not keep rebasing the PR?

@stoty
Copy link
Copy Markdown

stoty commented Feb 5, 2025

LGTM.

Note that in most cases the default jdbc:phoenix URL should be used, as the cluster can be determined from the hbase / hadoop config files that must be present on the classpath anyway for for correct operation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

revert

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The connection URL for Phoenix set with phoenix.connection-url supports multiple formats:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Connection URL:

Suggested change
### `phoenix.connection-url`
### Connection URL:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Change this whole list into an item list and wrap at 80 char width so we can actually review wording.

Like

* `jdbc:phoenix[:zk_quorum][:zk_port][:zk_hbase_path][:principal][:keytab][;options]`:
Connection uses HBase Zookeeper Registry. The `zk_quorum` is a comma separated
list of ZooKeeper servers. The `zk_port` is the ZooKeeper port. The
`zk_hbase_path` is the HBase root znode path, that is configurable using
`hbase-site.xml`.  By default the location is `/hbase`. Principal, Keytab and
Options are optional.
* `jdbc:phoenix+zk[:host1\:port1][,:host2\:port2]...[,:hostN\:portN][:zk_hbase_path][:principal][:keytab][;options]`:
Uses same Connection as above, however host:port pairs are separated by comma.
...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
| `phoenix.connection-url` | Yes | Detailed description: [](phoenix-connection-url). |
| `phoenix.connection-url` | Yes | See [](phoenix-connection-url). |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ideally we change this whole list into a list-table .. but that could be a separate commit before or after .. or even a separate PR

@virajjasani virajjasani force-pushed the reg branch 2 times, most recently from 9024a1a to d09425c Compare February 6, 2025 06:28
@virajjasani virajjasani requested a review from mosabua February 6, 2025 18:24
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove this line

Copy link
Copy Markdown
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Ready to go after the minor nits are fixed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
### Connection URL:
### Connection URL

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

remove one empty line

@mosabua mosabua merged commit 4011a7d into trinodb:master Feb 12, 2025
@github-actions github-actions bot added this to the 471 milestone Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants