gjacoby126 commented on a change in pull request #1079:
URL: https://github.com/apache/phoenix/pull/1079#discussion_r561272750
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixConfigurationUtil.java
##########
@@ -866,4 +872,18 @@ public static void setTenantId(Configuration
configuration, String tenantId){
configuration.set(MAPREDUCE_TENANT_ID, tenantId);
}
+ public static void setMRSnapshotManagedInternally(Configuration
configuration, Boolean isSnapshotRestoreManagedInternally) {
+ Preconditions.checkNotNull(configuration);
+ Preconditions.checkNotNull(isSnapshotRestoreManagedInternally);
+ configuration.set(MANAGE_MR_SNAPSHOT_RESTORE_INTERNALLY,
+ String.valueOf(isSnapshotRestoreManagedInternally));
+ }
+
+ public static boolean getMRSnapshotManagedInternally(final Configuration
configuration) {
Review comment:
In addition to switching the setting to Externally, this should probably
have an "is" prefix, not a get, because it's a boolean. (As you do for the
local variable)
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixConfigurationUtil.java
##########
@@ -866,4 +872,18 @@ public static void setTenantId(Configuration
configuration, String tenantId){
configuration.set(MAPREDUCE_TENANT_ID, tenantId);
}
+ public static void setMRSnapshotManagedInternally(Configuration
configuration, Boolean isSnapshotRestoreManagedInternally) {
Review comment:
I think should be setMRSnapshotManagedExternally. That keeps it closer
to the HBase solution this is modeled after (HBASE-18806) where true means that
the external job has already restored the snapshot and will clean it up.
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/iterate/TableSnapshotResultIterator.java
##########
@@ -88,19 +90,34 @@ public TableSnapshotResultIterator(Configuration
configuration, Scan scan, ScanM
}
private void init() throws IOException {
- RestoreSnapshotHelper.RestoreMetaChanges meta =
- RestoreSnapshotHelper.copySnapshotForScanner(this.configuration,
this.fs,
- this.rootDir, this.restoreDir, this.snapshotName);
- List<RegionInfo> restoredRegions = meta.getRegionsToAdd();
- this.htd = meta.getTableDescriptor();
- this.regions = new ArrayList<RegionInfo>(restoredRegions.size());
-
- for (RegionInfo restoredRegion : restoredRegions) {
- if (isValidRegion(restoredRegion)) {
- this.regions.add(restoredRegion);
+ if
(PhoenixConfigurationUtil.getMRSnapshotManagedInternally(configuration)) {
+ RestoreSnapshotHelper.RestoreMetaChanges meta =
+ RestoreSnapshotHelper.copySnapshotForScanner(this.configuration,
this.fs, this.rootDir,
+ this.restoreDir, this.snapshotName);
+ List<RegionInfo> restoredRegions = meta.getRegionsToAdd();
+ this.htd = meta.getTableDescriptor();
+ this.regions = new ArrayList<>(restoredRegions.size());
+ for (RegionInfo restoredRegion : restoredRegions) {
+ if (isValidRegion(restoredRegion)) {
+ this.regions.add(restoredRegion);
+ }
+ }
+ } else {
+ Path snapshotDir =
SnapshotDescriptionUtils.getCompletedSnapshotDir(snapshotName, rootDir);
Review comment:
@bharathv - I agree with your approach as the correct way forward, but
since we're very close to a release think that work can be deferred to a future
JIRA. The ability to turn off any automated snapshot handling is an easy fix to
the immediate problem, and still useful even after we fix the automation
problems later (for example if two jobs are part of a larger process and want
to share a restored snapshot.) What do you think?
@sakshamgangwar - if we do go with the current approach, we should probably
change the name of the JIRA and commit to something more descriptive of what it
actually does.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
