[GitHub] [phoenix] bharathv commented on a change in pull request #1079: Phoenix-6273: All the map tasks should operate on the same restored snapshot

[GitBox <gi...@apache.org>]

bharathv commented on a change in pull request #1079:
URL: https://github.com/apache/phoenix/pull/1079#discussion_r560490725



##########
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:
       Is the idea that the creator of this MR job restores this snapshot? If yes, I think that'd be inconvenient. How about we do it in the setup and let the InputFormat handle it? All user has to do is pass the snapshot name. Is there some issue with this approach?

##########
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)) {

Review comment:
       Not sure I get this, We are again restoring per iterator which is per scan? Isn't this what caused the issue in the first place. I see that it is guarded by a configuration option but ideally we want to avoid this in all cases right? or to rephrase my question, who wants to set this to true?




----------------------------------------------------------------
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:
users@infra.apache.org