github-actions[bot] commented on code in PR #67770:
URL: https://github.com/apache/doris/pull/67770#discussion_r4018143612
##########
fe/fe-core/src/main/java/org/apache/doris/backup/BackupHandler.java:
##########
@@ -237,6 +245,10 @@ public void alterRepository(String repoName, Map<String,
String> newProps)
if (oldRepo == null) {
throw new DdlException("Repository does not exist");
}
+ if (oldRepo.getUnavailableReason() != null) {
Review Comment:
[P2] Allow ALTER to repair descriptor-present unavailable repositories
`gsonPostProcess()` deliberately keeps `fileSystemDescriptor` when binding
its old properties fails, but this unconditional check returns before
`mergeProperties()` can apply the user's correction. A persisted S3/Azure
repository whose endpoint, credentials, or other formerly accepted property
becomes invalid after an upgrade is therefore impossible to repair with the
supported `ALTER REPOSITORY`; reconstructing the same replacement below would
validate the merged properties safely. Please reject only descriptor-null
legacy/corrupt records here, and cover a load-time bind failure followed by a
successful corrective ALTER.
##########
fe/fe-core/src/main/java/org/apache/doris/backup/Repository.java:
##########
@@ -251,36 +271,98 @@ 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 : "";
+ // Both binds run plugin code (bindPrimary probes every loaded
provider) at image load and
+ // edit-log replay: LinkageError included, so a half-installed
plugin costs this
+ // repository, not the FE - the same catch CatalogFactory uses for
catalogs.
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());
+ } catch (RuntimeException | LinkageError e) {
+ // A typed legacy record names its storage type ("S3", "HDFS",
"AZURE", ...); a broker
+ // record names its broker. The descriptor carries an explicit
type and is persisted by
+ // the next checkpoint, so a wrong guess here is permanent: a
typed record must never
+ // become BROKER because its plugin merely failed to load.
When the name is a shipped
+ // provider's and that provider is not loaded, its absence
explains why nothing claims
+ // the properties, whatever brokers are registered (a broker
may legally be called
+ // HDFS). Only a name that is not an absent provider's and is
a registered broker's is
+ // read as a broker record. Anything else is kept as it was,
so the migration is
+ // retried at the next start, and every use reports the reason
until then.
+ if (StorageRegistry.Provider.byName(fsName).isPresent() &&
!StorageAdapter.hasProvider(fsName)) {
Review Comment:
[P1] Do not convert provider failures into broker repositories
This condition only protects an *absent* typed provider. If a loaded
HDFS/S3/etc. provider throws `LinkageError` (or another bind failure) inside
`StorageAdapter.of()`, `hasProvider(fsName)` is true; when a legal broker has
the same name, the next condition is also false and this typed legacy record is
persisted as BROKER. The added tests cover the same-name collision and a
throwing provider separately, but their conjunction takes this path and the
next checkpoint loses the original type permanently. Please distinguish a clean
'no provider claimed the properties' result from provider execution/binding
failure, and leave the legacy record untouched on the latter.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/lineage/LineageEventProcessor.java:
##########
@@ -218,8 +220,12 @@ pluginRoots, getClass().getClassLoader(),
plugins.add(plugin);
LOG.info("Loaded lineage plugin: {}, pluginPath={}",
pluginName, props.get("plugin.path"));
}
- } catch (Exception e) {
+ } catch (Exception | LinkageError e) {
+ // create() and initialize() are the first calls into the
plugin implementation (the
+ // factory only named it): a dependency it lacks arrives here
as NoClassDefFoundError,
+ // which must cost this plugin alone, not FE startup. Its
classloader goes with it.
LOG.warn("Failed to create/initialize lineage plugin: {}",
pluginName, e);
+ runtimeManager.discard(pluginName);
Review Comment:
[P2] Roll back every published lineage-plugin state on init failure
At this point the external factory was already inserted into `factories` and
registered in `PluginRegistry`, while `discard()` only removes the runtime
handle and closes its URLClassLoader. The factory therefore still strongly
retains that loader, `information_schema.extensions` still advertises a plugin
whose create/initialize failed, and if `create()` returned an instance before
`initialize()` threw, that instance is never closed. The new test throws
directly from `create()` and only checks `hasActivePlugins()`, so it misses
these leaks. Please make admission transactional (or remove the
factory/registry row and close any created instance here) and test an
initialize-time failure.
##########
fe/fe-core/src/main/java/org/apache/doris/fs/FileSystemDescriptor.java:
##########
@@ -80,7 +80,13 @@ public StorageBackend.StorageType getThriftStorageType() {
* by BE to initialize its storage client for snapshot/upload/download
tasks.
*/
public Map<String, String> getBackendConfigProperties() {
- return StorageAdapter.of(properties).getBackendConfigProperties();
+ // A BROKER descriptor carries the raw WITH BROKER properties, which
routing does not claim
+ // (ofBroker documents that it bypasses routing): bind it the way it
was bound, or the first
+ // BACKUP on a broker repository throws here out of the job loop.
+ StorageAdapter adapter = storageType == FsStorageType.BROKER
+ ? StorageAdapter.ofBroker(name, properties)
Review Comment:
[P2] Contain broker-provider failure before task construction
Persisted BROKER descriptors skip binding in `Repository.gsonPostProcess()`,
so if the BROKER filesystem plugin is missing or fails binding after restart,
`unavailableReason` remains null. A journaled backup in `UPLOAD_SNAPSHOT` can
then rebind this repository, pass `getBrokerAddress()` because the broker
daemon still exists, and throw here from `StorageAdapter.ofBroker()` while
constructing its upload task (restore has the same path). The outer daemon
catch only logs, leaving the job state/status unchanged and retrying until
timeout. Please validate/mark broker descriptors unavailable at load, or
convert this bind failure into the job's normal repository `Status`, and cover
resumed upload/download with an unavailable BROKER provider.
##########
fe/fe-core/src/main/java/org/apache/doris/backup/Repository.java:
##########
@@ -251,36 +271,98 @@ 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 : "";
+ // Both binds run plugin code (bindPrimary probes every loaded
provider) at image load and
+ // edit-log replay: LinkageError included, so a half-installed
plugin costs this
+ // repository, not the FE - the same catch CatalogFactory uses for
catalogs.
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());
+ } catch (RuntimeException | LinkageError e) {
+ // A typed legacy record names its storage type ("S3", "HDFS",
"AZURE", ...); a broker
+ // record names its broker. The descriptor carries an explicit
type and is persisted by
+ // the next checkpoint, so a wrong guess here is permanent: a
typed record must never
+ // become BROKER because its plugin merely failed to load.
When the name is a shipped
+ // provider's and that provider is not loaded, its absence
explains why nothing claims
+ // the properties, whatever brokers are registered (a broker
may legally be called
+ // HDFS). Only a name that is not an absent provider's and is
a registered broker's is
+ // read as a broker record. Anything else is kept as it was,
so the migration is
+ // retried at the next start, and every use reports the reason
until then.
+ if (StorageRegistry.Provider.byName(fsName).isPresent() &&
!StorageAdapter.hasProvider(fsName)) {
+ unavailableReason = "legacy record of storage type '" +
fsName + "' was not migrated:"
+ + " its filesystem provider is not loaded (" +
e.getMessage() + ")";
+ } else if (!isRegisteredBroker(fsName)) {
Review Comment:
[P2] Recover this legacy broker repository after ADD BROKER
If the legacy broker name is absent during image load, this branch leaves
the descriptor null and sets `unavailableReason`. A later live or replayed `ADD
BROKER` only updates `BrokerMgr`; `ping()`, I/O, and ALTER all return early on
that retained reason, so the otherwise valid repository stays unusable until
another FE restart. Descriptor-backed broker repositories already resolve
BrokerMgr dynamically per I/O. Please retry this specifically classified legacy
migration when the broker becomes registered (or notify repositories from
broker ADD), clear the transient failure, and test absent-at-load followed by
ADD without restart.
--
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]
