Support customization of Iceberg connector name#15407
Support customization of Iceberg connector name#15407jitheshtr wants to merge 19 commits intotrinodb:masterfrom
Conversation
When refresh token is retrieved for UI, currently we were sending HTTP Status 303, assuming that all the request will just repeat the call on the Location header. When this works for GET/PUT verbs, it does not for non-idempotent ones like POST, as every js http client should do a GET on LOCATION after 303 on POST. Due to that I change it to 307, that should force every client to repeat exactly the same request, no matter the verb. Co-authored-by: s2lomon <s2lomon@gmail.com>
Actual work is done in `pageProjectWork.process()` call while `projection.project` only performs setup of projection. So both `expressionProfiler` and `metrics.recordProjectionTime` needed to be around that method.
Removes outdated comments and unnecessary methods in local exchange PartitioningExchanger since the operator is no longer implemented in a way that attempts to be thread-safe.
- Change ColumnHandle to BigQueryColumnHandle in BigQueryTableHandle - Extract buildColumnHandles in BigQueryClient
IcebergConnectorFactory allows customization by accepting additional module class to be bound in its constructor. While using this facility, we would also want to customize the connector name to signify that this is not the regular "iceberg" connector. Supporting an alternate name will also allow us to have both "iceberg" and "iceberg-custom" connector plugins installed together.
findepi
left a comment
findepi
left a comment
There was a problem hiding this comment.
IcebergConnectorFactory allows customization by accepting additional module class to be bound in its constructor.
This is there primarily for test purposes, not meant as extension point to create derivative connectors.
If you want a different, not-really-an-iceberg-connector, please do not use IcebergConnectorFactory (or override getName). In either case, you're doing this at your own risk, as the project won't guarantee any source/binary compatibility for that.
|
@findepi Thanks for the feedback. My understanding was that it's generally okay to be able to add additional modules through this extension e.g. for hive connector. And then it's possible add extensions for things that are bound using optional binder e.g. In this particular case, the end goal is for us is to be able to potentially use a different catalog implementation that follows the w.r.t. naming, I think we could have a separate factory that still makes use of InternalIcebergConnectorFactory, and have a different name. I guess we need to take the risk of it breaking with future changes - given this is not SPI. hive connector also allows adding a name. If that's for legacy reason, should we add a comment there saying it shouldn't be used as a precedent? |
makes sense to me
i think this is only to support the legacy name ( |
|
@jitheshtr there was apparently a force push to the master branch (per #15365 (comment)), so your PR appears as if containing some unrelated commits. You would need to rebase to remove them. However, I still think we shouldn't merge this change. |
|
having a different naming isn't a hard blocker imo - we can close this. |
|
Closing this based on the discussion above |
15 participants
Description
IcebergConnectorFactory allows customization by accepting additional module class to be bound in its constructor. While using this facility, we would also want to customize the connector name to signify that this is not the regular "iceberg" connector. Supporting an alternate name will also allow us to have both "iceberg" and "iceberg-custom" connector plugins installed together.
Additional context and related issues
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: