Skip to content

[AMORO-4232][common] Improve AmsThriftUrl parsing and validation - #4372

Open
sivakumar-mahalingam wants to merge 1 commit into
apache:masterfrom
sivakumar-mahalingam:fix/4232-ams-thrift-url
Open

sivakumar-mahalingam wants to merge 1 commit into
apache:masterfrom
sivakumar-mahalingam:fix/4232-ams-thrift-url

Conversation

@sivakumar-mahalingam

Copy link
Copy Markdown
Contributor

Why are the changes needed?

AmsThriftUrl lowercased the complete input URL during parsing, which could modify case-sensitive components such as catalog paths and query parameter values. Thrift URL validation also did not clearly handle unsupported schemes, missing hosts, or missing/invalid ports.

Fix #4232.

Brief change log

  • Preserve the original case of hosts, catalog paths, query parameter values, and URLs.
  • Compare thrift and ZooKeeper schemes case-insensitively.
  • Add validation for thrift scheme, host, and port with clearer error messages.
  • Consolidate duplicated socketTimeout query parsing.
  • Keep existing public APIs unchanged.
  • Add unit tests for valid and invalid thrift and ZooKeeper URL scenarios.

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before making a pull request

The following tests were run locally:

.\mvnw.cmd -pl amoro-common -Dtest=TestAmsThriftUrl test

Result:

Tests run: 9, Failures: 0, Errors: 0, Skipped: 0

------------------------------------------------------------------------
BUILD SUCCESS
------------------------------------------------------------------------

The tests cover:

  • Mixed-case thrift schemes.
  • Lowercase thrift URLs.
  • Case preservation for URL components.
  • socketTimeout parsing.
  • Unsupported schemes.
  • Missing hosts.
  • Missing ports.
  • Invalid ports.
  • ZooKeeper URL and timeout handling.

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improvement]: Improve AmsThriftUrl parsing and validation

1 participant