ndimiduk commented on code in PR #6294:
URL: https://github.com/apache/hbase/pull/6294#discussion_r1802659801
##########
hbase-protocol-shaded/src/main/protobuf/Backup.proto:
##########
@@ -54,11 +54,12 @@ message TableServerTimestamp {
/**
* Structure keeps relevant info for backup restore session
+ * backup_root_dir is deprecated because we no longer store root dirs on disk
HBASE-28882
*/
message BackupImage {
optional string backup_id = 1;
optional BackupType backup_type = 2;
- optional string backup_root_dir = 3;
+ optional string backup_root_dir = 3 [deprecated = true];
Review Comment:
Sorry, we have to keep it around for a bit. Someone out there upgrading from
2.6.0 will have backup manifests on disk that they will presumably want to
restore from. If this wasn't for data at rest, I think that we could fall back
on the "this feature is beta" path, but it's not that painful to follow the
process here.
All that said, the requirement is that a user of hbase 2.6.1+ can restore
backups made using hbase 2.6.0. So long as the presence of the field doesn't
cause an error, and the backup can still be used to restore, i think you're
good to ignore the presence of the field going forward. In 2.6.1+ you should
check for the presence of the field and log a warning that this backup will not
be readable from an HBase of that future version.
The deprecation/removal process is described in [the
book](https://hbase.apache.org/book.html#hbase.versioning.post10) under the
subheading "Client API compatibility". This isn't technically a client-facing
API, but it's data at rest. In this case, we'll need to keep the field around
and functional, marked as deprecated, for all the branch-2 and branch-3 release
lines, and we can remove it from master (4.x).
So, you can completely drop the field from this PR vs. master but it must
stay for all the backports.
If you can find some other process describing a more accelerated pace of
changes to a "beta" feature, I'm all ears.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/HBackupFileSystem.java:
##########
@@ -97,10 +100,18 @@ public static Path getTableBackupPath(TableName tableName,
Path backupRootPath,
private static Path getManifestPath(Configuration conf, Path backupRootPath,
String backupId)
throws IOException {
+ return getManifestPath(conf, backupRootPath, backupId, true);
+ }
+
+ /* Visible for testing only */
+ @RestrictedApi(explanation = "Should only be called internally or in tests",
link = "",
Review Comment:
Why the RestrictedApi stuff? this is an IA.Private class and the method is
package-protected. That seems enough. Same case for the usage in BackupManifest.
##########
hbase-protocol-shaded/src/main/protobuf/Backup.proto:
##########
@@ -54,11 +54,12 @@ message TableServerTimestamp {
/**
* Structure keeps relevant info for backup restore session
+ * backup_root_dir is deprecated because we no longer store root dirs on disk
HBASE-28882
Review Comment:
We have a kind of standard language to decorate code that is participating
in a deprecation cycle.
> This foo was marked as deprecated in HBase x.y.z, will be removed in
(x+2).0.0.
--
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]
