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 support for custom socket factory #1400

Conversation

mayankvadariya
Copy link

Summary

#1391 - Attempt to inject a custom socket factory with APACHE_HTTP_CLIENT connection provider.

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG
  • For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

@CLAassistant
Copy link

CLAassistant commented Jul 12, 2023

CLA assistant check
All committers have signed the CLA.

@mayankvadariya
Copy link
Author

@zhicwu I attempted this PR to add support for custom socket factory. Please review the approach and advise for any changes. Thank you!

@mayankvadariya mayankvadariya changed the title Mayank/support custom socket factory Add support for custom socket factory Jul 12, 2023
@mayankvadariya mayankvadariya force-pushed the mayank/support-custom-socket-factory branch from 8ec7710 to fdb56ef Compare July 12, 2023 23:26
@zhicwu
Copy link
Contributor

zhicwu commented Jul 12, 2023

Thank you @mayankvadariya for the pull request! Just some immediate feedback I can think of:

  1. Please consider to use more generic names for the new options, for examples: CUSTOM_SOCKET_FACTORY & CUSTOM_SECURE_SOCKET_FACTORY, so that later they can be used in clickhouse-tcp-client as well. Alternatively, we can move the options to ClickHouseHttpOption (better without the http* prefix), so that it's clear that they're just for clickhouse-http-client.
  2. Since we're adding new options, in case user wants to use their own extension in an environment like DBeaver, perhaps it's better to try context class loader first, and then parent class loader for loading the custom factory class
  3. Does it make sense to reuse existing socket options instead of additional ones for custom socket factory?

@mayankvadariya mayankvadariya force-pushed the mayank/support-custom-socket-factory branch from fdb56ef to beb77c3 Compare July 13, 2023 22:24
@mayankvadariya
Copy link
Author

  1. Aptly said. Moved the options to ClickHouseHttpOption
  2. Please elaborate more on this.
  3. It's more about scoping input properties to ClickHouseOption, unknown/unparsed properties are being ignored through driver. Either we can add a way to allow accessing unknown/unparsed properties through ClickHouseOption or add these options. This way custom socket factory doesn't have to depend on the supported parameters by the Clickhouse drivers. Most of the drivers allows access to unknown/unparsed properties when creating a custom socket factory.
    I am also up for getting rid of both options(*_OPTIONS) by making unknown/unparsed input properties somehow available through ClickHouseConfig.

@zhicwu
Copy link
Contributor

zhicwu commented Jul 14, 2023

  1. Please elaborate more on this.

Assume we're using DBeaver, when loading a class, I was thinking maybe it's better to start with the context class loader.
Anyway, let's not worry about this for now. I need to think it through and then probably come up with a separate PR later for the change.

image

  1. It's more about scoping input properties to ClickHouseOption, unknown/unparsed properties are being ignored through driver. Either we can add a way to allow accessing unknown/unparsed properties through ClickHouseOption or add these options. This way custom socket factory doesn't have to depend on the supported parameters by the Clickhouse drivers. Most of the drivers allows access to unknown/unparsed properties when creating a custom socket factory.
    I am also up for getting rid of both options(*_OPTIONS) by making unknown/unparsed input properties somehow available through ClickHouseConfig.

Good point. Perhaps we just need an extra field in ClickHouseConfig for unknown properties. Will make some changes this weekend.

@mayankvadariya
Copy link
Author

@zhicwu Did you get any chance to add extra config for getting unknown properties?

@zhicwu
Copy link
Contributor

zhicwu commented Jul 18, 2023

@zhicwu Did you get any chance to add extra config for getting unknown properties?

Sorry I need someone from ClickHouse team to merge #1403 first to unblock my work, and then I'll be able to make the change and merge other PRs. Let me ping them on Slack.

@mayankvadariya
Copy link
Author

@zhicwu we are waiting on this to get merged at the earliest. Do you have any timeline on when this PR might get merged?

@anusudarsan
Copy link

@zhicwu any progress on the blocker here?

@zhicwu
Copy link
Contributor

zhicwu commented Aug 1, 2023

@zhicwu any progress on the blocker here?

Hi @anusudarsan, sorry for the delay. I had meeting with ClickHouse team on Monday - we'll gradually drop gRPC client but keep it for now. I'll fix CI issue and get back to this tonight.

@mzitnik
Copy link
Contributor

mzitnik commented Aug 2, 2023

@zhicwu any progress on it?

@mayankvadariya
Copy link
Author

@zhicwu Checking for the progress on the blockers.

@zhicwu
Copy link
Contributor

zhicwu commented Aug 9, 2023

Sorry for being super lazy - I was focusing on something else 😂

The initial CI failure has been addressed, but this PR is still failing compilation likely due to moving classes to the http client module. Regarding supporting unknown properties, I plan to complete that this weekend along with two other changes:

  1. Stop printing properties in exceptions
  2. Support properties with defaults (e.g. new Properties(defaultProps))

@mayankvadariya mayankvadariya force-pushed the mayank/support-custom-socket-factory branch from beb77c3 to c9f0142 Compare August 9, 2023 14:20
@mayankvadariya
Copy link
Author

Thanks @zhicwu for checking. I replaced string.formatted with string.format as the CI builds with java8. Please trigger the workflow again.

@mayankvadariya mayankvadariya marked this pull request as ready for review August 11, 2023 13:21
@zhicwu
Copy link
Contributor

zhicwu commented Aug 14, 2023

Hi @mayankvadariya, over the weekend, I gave this some thought and believe it would be better to create a generic interface to define custom socket factories. This would allow support for not just Apache HTTP Client but other implementations as well. I've created a separate PR for this approach - see #1420. Please let me know if you have any concerns with this proposal.

@zhicwu
Copy link
Contributor

zhicwu commented Aug 28, 2023

Close as this will be implemented in #1420.

@zhicwu zhicwu closed this Aug 28, 2023
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.

5 participants