gjacoby126 commented on a change in pull request #1079:
URL: https://github.com/apache/phoenix/pull/1079#discussion_r555318649
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/iterate/TableSnapshotResultIterator.java
##########
@@ -164,9 +165,6 @@ public void close() throws SQLException {
closed = true; // ok to say closed even if the below code throws an
exception
try {
scanIterator.close();
- fs.delete(this.restoreDir, true);
Review comment:
@sakshamgangwar - It seems correct that if we're not restoring the
snapshot in the iterator anymore that we can't clean it up there either. But it
still seems like we need to clean up somewhere.
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/iterate/MapReduceParallelScanGrouper.java
##########
@@ -71,6 +72,9 @@ public boolean shouldStartNewScan(QueryPlan plan, List<Scan>
scans,
FileSystem fs = rootDir.getFileSystem(conf);
Path snapshotDir =
SnapshotDescriptionUtils.getCompletedSnapshotDir(snapshotName, rootDir);
SnapshotDescription snapshotDescription =
SnapshotDescriptionUtils.readSnapshotInfo(fs, snapshotDir);
+ //Performing snapshot restore which will be
used during scans
Review comment:
A bit strange for a function called getRegionBoundaries to have side
effects, especially when it won't in the non-MR case. I don't see an obvious
better place to put this, because the calling function is the same whether
we're doing MR or non-MR, I believe. But if we find a good place to do the
cleanup (see below), it might also be a better place to do this setup.
----------------------------------------------------------------
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]
