morningman commented on PR #69605:
URL: https://github.com/apache/doris/pull/69605#issuecomment-6413461490
Architecture follow-up for commit
`719de6ac81f0abe4889f8b43d8752edc4f927ebd`. The typed configuration,
centralized input conversion, and holder snapshots improve the structure. The
following suggestions focus on making the result clearer, simpler, and easier
to extend.
**Main relationships after the refactor**
```text
properties / Thrift / PB
|
Resolver
|
ObjStorageConfig
+-- bucket / prefix: storage target
+-- ObjClientConfig: configuration needed to create a client
|
Factory
|
S3 / Azure Provider
|
Cache / SDK
|
ObjStorageClient
Configuration refresh -> merge update -> Holder publishes configuration and
client together
```
**1. Consolidate the common creation flow and keep providers focused on SDK
construction.**
The [S3
provider](https://github.com/apache/doris/blob/719de6ac81f0abe4889f8b43d8752edc4f927ebd/be/src/io/obj_storage/s3_client_provider.cpp#L40)
and [Azure
provider](https://github.com/apache/doris/blob/719de6ac81f0abe4889f8b43d8752edc4f927ebd/be/src/io/obj_storage/azure_client_provider.cpp#L75)
both follow essentially the same sequence:
```text
cache lookup -> build -> rate-limit wrapping -> cache publication
```
They also call the factory's rate-limit helper, creating a dependency that
goes `Factory -> Provider -> Factory`.
Consider having the factory own the common creation flow and cache, while
each provider only builds its backend client. The existing cache key already
includes the backend, so it can distinguish clients for different backends.
This would reduce duplication and make the dependency direction more direct.
The tradeoff is sharing one short cache-lock critical section across the two
backends. If separate caches are preferable, extracting a small common creation
helper would still remove the duplicated sequence. A complex provider
inheritance hierarchy is unnecessary here.
**2. Keep one implementation of typed configuration validation.**
The resolver's
[validate_flat_fields](https://github.com/apache/doris/blob/719de6ac81f0abe4889f8b43d8752edc4f927ebd/be/src/io/obj_storage/obj_storage_config_resolver.cpp#L104)
and the factory's [S3
validation](https://github.com/apache/doris/blob/719de6ac81f0abe4889f8b43d8752edc4f927ebd/be/src/io/obj_storage/obj_client_factory.cpp#L62)
repeat checks for the endpoint, region, credential combinations, and
access/secret keys.
It would help to distinguish:
- Input handling: parsing strings, extracting URI/bucket information, and
preserving legacy properties requirements.
- Configuration semantics: whether a typed `S3ClientConfig` or
`AzureClientConfig` is valid.
The second group should have one implementation, always called by the
factory and reused by the resolver when early validation is needed. Adding an
authentication mode would then be less likely to leave two sets of rules
inconsistent.
Preserve existing compatibility rules during this cleanup. For example,
Azure properties still require a region today; reorganizing validation should
not silently change that behavior.
**3. Extend compile-time variant checking to configuration merging and
authentication.**
The factory's backend dispatch already uses `std::visit`, which is useful.
Some other paths still assume there are only two alternatives:
- The `else` branch in
[merge_update](https://github.com/apache/doris/blob/719de6ac81f0abe4889f8b43d8752edc4f927ebd/be/src/io/obj_storage/obj_storage_config_resolver.cpp#L431)
implicitly means Azure.
- [Azure
validation](https://github.com/apache/doris/blob/719de6ac81f0abe4889f8b43d8752edc4f927ebd/be/src/io/obj_storage/obj_client_factory.cpp#L100)
and
[construction](https://github.com/apache/doris/blob/719de6ac81f0abe4889f8b43d8752edc4f927ebd/be/src/io/obj_storage/azure_client_provider.cpp#L113)
directly extract `AzureSharedKeyConfig`.
Adding a third backend or an Azure OAuth alternative would not necessarily
expose every missing case at compile time. Some omissions could instead become
a runtime `std::bad_variant_access`.
Consider explicit per-alternative visitor overloads for these paths too.
Pure configuration merging could also live in the configuration layer, so the
holder's refresh logic does not need to depend on the input resolver. Both
changes would make extension points more explicit.
**4. Make the scope of shared connection options match their actual effect.**
[ObjStorageConnectionOptions](https://github.com/apache/doris/blob/719de6ac81f0abe4889f8b43d8752edc4f927ebd/be/src/io/obj_storage/obj_storage_config.h#L44)
appears to apply to every backend. However, Azure construction receives only
`AzureClientConfig`: the three connection options currently have no effect
there, while still participating in cache identity.
This inherits the old behavior and is not a new regression. The new model
nevertheless gives callers a broader impression of support than the
implementation provides.
A small first step would be to document that these options currently apply
only to S3. A later change could move them into S3 configuration or implement
explicit Azure equivalents. Adding Azure support should be treated as a
behavior change with corresponding tests.
**5. As a lower-priority cleanup, align the remaining S3ClientFactory name
with its responsibilities.**
The retained
[S3ClientFactory](https://github.com/apache/doris/blob/719de6ac81f0abe4889f8b43d8752edc4f927ebd/be/src/util/s3_util.h#L62)
no longer creates object-storage clients. Its main responsibilities are SDK
lifetime, AWS helpers, and CA handling; Azure still obtains CA information
through it.
At minimum, a more accurate name would avoid suggesting that the code now
has two client factories. If CA handling is later extracted into a
backend-neutral utility, keep SDK lifetime ownership explicit and preserve the
guarantee that clients are destroyed before SDK shutdown.
I would retain the typed configuration, centralized input conversion, and
independent holder snapshots. Prioritizing points 1-3 would further reduce
duplicated rules and dependency cycles, while concentrating the changes
required to add a backend and making omissions easier for the compiler to
detect.
Validation was source-based; no builds or tests were run for this
architecture analysis.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
