JoaoJandre commented on PR #12758: URL: https://github.com/apache/cloudstack/pull/12758#issuecomment-4137810582
Hello @abh1sar, > HI @JoaoJandre About the name, I think functionality wise what distinguishes this provider from an existing KVM backup provider (NAS) is the use of secondary storage. So it makes some sense to use secondary storage in the name. So, probably something like Secondary Storage Backup Provider (BackupProviderPlugin value can be `secstore` in short) or KVM Secondary Storage Backup Provider ( BackupProviderPlugin value can be `kvmsecstore` in short). This seems more logical to me, but I don't have a hard preference on this matter. Thank you for the suggestions. Function-wise, there are several other distinctions from NAS that I think are worth considering for the name: - Incremental backups - Compression - Validation - Quick restore The secondary storage aspect is an important implementation detail, but I'm not sure it needs to be in the provider name itself. Adding all distinguishing features to the name would obviously become cumbersome. I would prefer something more generic like `ACS KVM Backup Engine`. > Will you be changing the term native/knib in the class files and configurations as well? Yes, once we settle on a name, I'll update the code accordingly. I believe the term "native" has already been removed from configurations and APIs in the latest version. > Can I ask you to move all the files that don't have a strict dependency to be in the server module to plugins/backup/ ? Adding provider specific files in the server module is not really a good model if that can be avoided. > > server/src/main/java/org/apache/cloudstack/backup/BackupCompressionServiceJobController.java server/src/main/java/org/apache/cloudstack/backup/BackupValidationServiceController.java server/src/main/java/org/apache/cloudstack/backup/NativeBackupServiceImpl.java server/src/main/java/org/apache/cloudstack/backup/NativeBackupServiceJobController.java Unfortunately, these files have dependencies that prevent moving them to the plugin module. However, I wrote them with extensibility in mind. Future backup providers can extend and reuse these features. For example, `NativeBackupServiceImpl` discovers "native" providers at startup and delegates to them. If another provider extends `NativeBackupProvider` and implements the required methods, the framework should work for them as well. Here the term "Native" refers to a class of backup providers which could implement these features. I'm happy to rename the term to something else for clarity, like "Internal". Since these class names are internal to the codebase and not exposed to users, the rename should be straightforward. > Also, what are these methods in NativeBackupServiceImpl for? I don't see a caller in the code. `orchestrateRestoreVMFromBackup` `orchestrateDeleteBackup` `orchestrateTakeBackup` `orchestrateRestoreBackupVolumeAndAttachToVM` These methods are invoked via reflection, they have the `@ReflectionUse` annotation. There are similar examples elsewhere in the codebase (for instance, in `VolumeApiServiceImpl`) where methods are called through reflection rather than directly. -- 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]
