deniskuzZ commented on code in PR #6789:
URL: https://github.com/apache/hive/pull/6789#discussion_r4400138483
##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergStorageHandler.java:
##########
@@ -279,8 +279,23 @@ public HiveAuthorizationProvider
getAuthorizationProvider() {
@Override
public void configureInputJobCredentials(TableDesc tableDesc, Map<String,
String> secrets) {
- RestCatalogScanPlanningUtil.propagateCatalogSecretsToJob(tableDesc,
secrets, conf);
- IcebergVendedCredentialUtil.propagateVendedCredentialsToJob(tableDesc,
secrets, conf);
+ Properties props = tableDesc.getProperties();
+ if (props == null) {
+ return;
+ }
+ if (!IcebergVendedCredentialUtil.requestsVendedCredentials(props, conf) &&
+ !RestCatalogScanPlanningUtil.isServerSideScanPlanningEnabled(conf)) {
+ return;
+ }
+ Table table;
+ try {
+ table = IcebergVendedCredentialUtil.getTableWithVendedCredentials(props,
conf);
+ } catch (NoSuchTableException e) {
+ // Table may not exist yet for CTAS; there is nothing to plan or vend
credentials for.
+ return;
+ }
+ RestCatalogScanPlanningUtil.propagateCatalogSecretsToJob(table, tableDesc,
secrets, conf);
Review Comment:
Could we simplify how credentials reach the job? Right now
`configureInputJobCredentials` passes `table` + `tableDesc` into two helpers
that each re-check null props, re-evaluate their own gate, and read
`CATALOG_NAME` (one with the session-default fallback, one raw). Underneath,
`propagateToJob` takes two out-param maps and callers pass `null` to select a
half.
Proposal:
**1. Return both halves instead of writing into nullable maps**
```java
/** Vended credentials split per HIVE-20651: plain job properties vs job
secrets. */
record VendedJobConf(Map<String, String> properties, Map<String, String>
secrets) {
static final VendedJobConf EMPTY = new VendedJobConf(Map.of(), Map.of());
}
static VendedJobConf toJobConf(Table table, String catalogName,
Configuration conf) {
List<StorageCredential> credentials =
withConfigurationOverrides(catalogName, extractCredentials(table),
conf);
if (credentials.isEmpty()) {
return VendedJobConf.EMPTY;
}
Map<String, String> properties = new LinkedHashMap<>();
Map<String, String> secrets = new LinkedHashMap<>();
secrets.put(
InputFormatConfig.vendedCredentialsKey(table.name()),
serializeToSingleLineBase64(Lists.newArrayList(credentials)));
for (StorageCredential credential : credentials) {
addCredentialEntries(catalogName, credential, properties, secrets, conf);
}
return new VendedJobConf(properties, secrets);
}
```
The `!= null` checks in `addCredentialEntries` / `addCredentialEntry` go
away, and `propagateToJob` / `propagateVendedCredentialsToJob` can be dropped.
**2. Let `configureInputJobCredentials` own the decisions**
```java
@Override
public void configureInputJobCredentials(TableDesc tableDesc, Map<String,
String> secrets) {
Properties props = tableDesc.getProperties();
if (props == null) {
return;
}
boolean vendedCredentials =
IcebergVendedCredentialUtil.requestsVendedCredentials(props, conf);
if (!vendedCredentials &&
!RestCatalogScanPlanningUtil.isServerSideScanPlanningEnabled(conf)) {
return;
}
Table table;
try {
table = IcebergVendedCredentialUtil.getTableWithVendedCredentials(props,
conf);
} catch (NoSuchTableException e) {
// Table may not exist yet for CTAS; there is nothing to plan or vend
credentials for.
return;
}
String catalogName = HiveTableUtil.resolveCatalogName(conf,
props.getProperty(InputFormatConfig.CATALOG_NAME));
if (RestCatalogScanPlanningUtil.shouldPlanServerSide(conf, table)) {
RestCatalogScanPlanningUtil.propagateCatalogSecretsToJob(catalogName,
secrets, conf);
}
if (vendedCredentials) {
secrets.putAll(IcebergVendedCredentialUtil.toJobConf(table, catalogName,
conf).secrets());
}
}
```
with `propagateCatalogSecretsToJob` reduced to the copy itself:
```java
public static void propagateCatalogSecretsToJob(
String catalogName, Map<String, String> jobSecrets, Configuration
sessionConf) {
if (StringUtils.isEmpty(catalogName)) {
return;
}
String catalogPrefix = IcebergCatalogProperties.CATALOG_CONFIG_PREFIX +
catalogName + ".";
sessionConf.forEach(entry -> {
if (entry.getKey().startsWith(catalogPrefix)) {
jobSecrets.put(entry.getKey(), entry.getValue());
}
});
}
```
**3. Other call sites**
```java
// configureInputJobProperties
if (isVendedCredentials) {
IcebergVendedCredentialUtil.toJobConf(table, catalogName, configuration)
.properties().forEach(map::putIfAbsent);
table = IcebergVendedCredentialUtil.secretFreeCopy(table, configuration);
}
// refreshVendedCredentialsIfMissing
VendedJobConf vended = toJobConf(table, catalogName, configuration);
vended.properties().forEach(jobConf::set);
mergeJobSecrets(tableDesc, vended.secrets());
```
Here, `catalogName` should be resolved the same way in all three places
(`HiveTableUtil.resolveCatalogName`). Today the secrets path falls back to the
session default, the vended paths use the raw `CATALOG_NAME` (null for
default-catalog tables, so `withConfigurationOverrides` skips session
overrides), and `refreshVendedCredentialsIfMissing` returns early on null. Is
that difference intended?
Tests:
`TestRestCatalogScanPlanningUtil.propagateCatalogSecretsToJobSkipsTableNotPlannedServerSide`
would move to the storage handler (the gate now lives there), and the IT call
at `TestHiveIcebergServerSideScanPlanningServerIT:404` needs the new signature.
--
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]
