github-actions[bot] commented on code in PR #67770:
URL: https://github.com/apache/doris/pull/67770#discussion_r4022775480
##########
fe/fe-core/src/main/java/org/apache/doris/backup/Repository.java:
##########
@@ -809,14 +921,21 @@ public Status download(String remoteFilePath, String
localFilePath) {
return Status.OK;
} catch (FileNotFoundException e) {
return new Status(ErrCode.NOT_FOUND, "file " + localFilePath + "
does not exist");
- } catch (IOException e) {
+ } catch (IOException | LinkageError | ServiceConfigurationError e) {
return new Status(ErrCode.COMMON_ERROR, "Failed to download file:
" + e.getMessage());
} finally {
releaseSpiFs(fs);
}
}
public Status getBrokerAddress(Long beId, Env env, List<FsBroker>
brokerAddrs) {
+ if (unavailableReason != null) {
+ // The first repository call every backup and restore job makes
before it reads the
+ // descriptor's properties: a job on an unusable repository fails
here with a Status and is
+ // cancelled, instead of throwing out of run() on every tick.
Review Comment:
[P2] Validate task properties for non-broker repositories too
This guard is reached only after the non-BROKER early return, but
backend-property projection is a separate provider callback that
bind/create/ping never exercises. A non-BROKER provider can therefore serve FE
I/O and still return no `toBackendProperties()` value or link-fail there;
upload, restore, and cloud-restore then throw while constructing their BE
tasks, and the daemon only logs/retries the unchanged job until timeout. ALTER
can likewise journal the new repository before active-task refresh throws.
Please validate/cache this projection for every repository type inside the
Status boundary (and before committing ALTER), then pass that validated map to
task construction.
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/DatasourcePrintableMap.java:
##########
@@ -101,6 +107,24 @@ public class DatasourcePrintableMap<K, V> extends
BasicPrintableMap<K, V> {
"cos.access_key", "cos.secret_key",
"obs.access_key", "obs.secret_key",
"minio.access_key", "minio.secret_key",
"minio.session_token"));
+ // The aliases the shipped filesystem plugins declare sensitive beyond
the legacy union above.
+ // Masking must not depend on a plugin having loaded: a shipped plugin
that fails to load is
+ // skipped and the FE serves on, while every catalog, repository and
vault created with its
+ // aliases stays in metadata and is printed by SHOW CREATE CATALOG,
SHOW CATALOG, the
Review Comment:
[P1] Preserve custom sensitive aliases when a plugin is rejected
The new degraded-startup path can now reject an out-of-tree provider and
continue into image replay, but that provider's `sensitivePropertyKeys()`
snapshot lived only in the previous JVM. This fallback restores shipped aliases
only, so a catalog created with a custom alias such as `acme.auth_blob` is
replayed after the plugin link failure and SHOW/SHOW CREATE prints the
credential verbatim to users with catalog SHOW privilege. This is distinct from
the lazy-set thread: admission succeeded in the old process and the provider is
wholly absent in the new one. Please persist a host-owned alias snapshot
independently of executable provider admission and restore it before printable
metadata becomes available, with a successful-create then rejected-on-restart
test.
##########
fe/fe-core/src/main/java/org/apache/doris/backup/Repository.java:
##########
@@ -251,36 +272,116 @@ public void gsonPostProcess() {
LOG.info("Repository '{}': migrating legacy 'fs' field to
'fs_descriptor'", name);
Map<String, String> props = legacyFs.properties != null ?
legacyFs.properties : new HashMap<>();
String fsName = legacyFs.name != null ? legacyFs.name : "";
- try {
- StorageAdapter storageAdapter = StorageAdapter.of(props);
- fileSystemDescriptor =
FileSystemDescriptor.fromStorageAdapter(storageAdapter, "");
- } catch (RuntimeException e) {
- LOG.warn("Repository '{}': primary storage migration failed
({}), trying broker fallback",
- name, e.getMessage());
+ // The record's name is the one identity it carries: a typed
legacy record names its storage
+ // type ("S3", "HDFS", "AZURE", ...), a broker record names its
broker. The descriptor written
+ // here carries an explicit type and is persisted by the next
checkpoint, so it has to be
+ // decided by that identity, not by whichever provider happens to
claim the properties: a
+ // WITH BROKER repository commonly stores fs.defaultFS and the
HDFS provider would claim it,
+ // and a typed repository must never become BROKER because its
plugin failed to load, threw,
+ // or rejected the properties. So: a storage type's name binds
that type, or is kept with the
+ // reason; a registered broker's name is a broker record; anything
else - a name from before
+ // the type names, or a broker since dropped - is routed as it
always was. A kept record is
+ // retried at the next start, and every use reports the reason
until then. Every bind runs
+ // plugin code at image load and edit-log replay, so a
LinkageError or a plugin's own
Review Comment:
[P1] Bind typed legacy records through their saved provider
This branch treats `fsName` as the authoritative storage type but then
ignores it by calling generic priority routing. For example, a valid legacy
`AZURE` record that also carries a stale `fs.s3.support=true` flag is claimed
by S3 first; the next line builds an S3 descriptor, and the next checkpoint
permanently makes backup/restore use S3 normalization and BE properties. This
is distinct from the broker-identity thread because the typed branch is already
selected and every bind succeeds. Please use `StorageAdapter.ofProvider(fsName,
props)` here and cover checkpoint/reload with properties that another provider
also claims.
--
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]
