suxiaogang223 commented on PR #62988: URL: https://github.com/apache/doris/pull/62988#issuecomment-6762549294
Local code review of `c23c4ab8d60099ecbd69eb8d8337acda87b0bc68`: one P2 regression remains. ### [P2] Preserve the Hadoop FileIO configuration for HMS catalogs using GCS HMAC Location: [IcebergCatalogFactory.java, lines 465–466](https://github.com/apache/doris/blob/c23c4ab8d60099ecbd69eb8d8337acda87b0bc68/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergCatalogFactory.java#L465-L466). This affects an existing Iceberg HMS catalog with `provider=GCP`, `gs.access_key` / `gs.secret_key`, metadata or manifest locations using the supported `s3a://` alias, and no explicit `io-impl`. Previously, HiveCatalog defaulted to HadoopFileIO and used the GCS endpoint and HMAC credentials in the Hadoop configuration. The new unconditional default selects NativeGcsResolvingFileIO for HMAC catalogs as well. That implementation pins only `gs://` to HadoopFileIO; `s3a://` still resolves to S3FileIO. The HMS branch deliberately skips `appendS3FileIO`, and HMAC does not enter the native-credential branch that calls `putS3FileIODialect`. Consequently, the new S3FileIO has none of `s3.endpoint`, `s3.access-key-id`, `s3.secret-access-key`, or `client.region`, although the corresponding Hadoop configuration is populated. ```text Before: HiveCatalog -> HadoopFileIO -> configured fs.s3a.* After: HiveCatalog -> NativeGcsResolvingFileIO -> S3FileIO -> missing catalog options ``` I reproduced the dispatch through the current property assembly and real Iceberg FileIO implementations in a local Java probe. With ambient AWS credentials/region disabled, the new path throws `SdkClientException: Unable to load region`, while the original HadoopFileIO path constructs a HadoopInputFile. Only the Hadoop transport was substituted in the probe; no real GCS connection was made. Please preserve the previous HadoopFileIO default for HMS HMAC catalogs, or supply the complete S3FileIO configuration when changing their default. Add coverage for actual FileIO dispatch with HMS + GCS HMAC + `s3a://`; the new non-native routing test currently covers REST only. ### Validation - `run-fe-ut.sh --run 'IcebergCatalogFactoryTest,IcebergConnectorValidatePropertiesTest,IcebergRestMetaStorePropertiesTest,IcebergPluginTest,IcebergSerializationCompatTest'`: **131 tests passed**, zero failures/errors/skips; the complete script finished successfully after removing stale FE core build artifacts from an earlier branch. - Checkstyle validation passed for the affected modules and their reactor dependencies. - A local mock OAuth/REST service verified ADC token refresh and the Bearer and `x-goog-user-project` headers on `/config` and namespace requests. - Real GCS access and the deployed plugin-directory dependency closure were not revalidated. The previously reported BE dependency gap is addressed in the current POMs and the serialization tests pass. The HMS routing regression should be fixed before marking this review PASS. -- 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]
