shahrs87 commented on a change in pull request #1079:
URL: https://github.com/apache/phoenix/pull/1079#discussion_r561910478
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixConfigurationUtil.java
##########
@@ -866,4 +871,18 @@ public static void setTenantId(Configuration
configuration, String tenantId){
configuration.set(MAPREDUCE_TENANT_ID, tenantId);
}
+ public static void setMRSnapshotManagedExternally(Configuration
configuration, Boolean isSnapshotRestoreManagedExternally) {
+ Preconditions.checkNotNull(configuration);
+ Preconditions.checkNotNull(isSnapshotRestoreManagedExternally);
+ configuration.set(MANAGE_MR_SNAPSHOT_RESTORE_EXTERNALLY,
+ String.valueOf(isSnapshotRestoreManagedExternally));
+ }
+
+ public static boolean isMRSnapshotManagedExternally(final Configuration
configuration) {
+ Preconditions.checkNotNull(configuration);
+ boolean isSnapshotRestoreManagedInternally =
Review comment:
Typo in the variable name: `isSnapshotRestoreManagedInternally`. Should
it be isSnapshotRestoreManaged*Ex*ternally ?
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixConfigurationUtil.java
##########
@@ -191,6 +190,12 @@
// provide an absolute path to inject your multi input mapper logic
public static final String MAPREDUCE_MULTI_INPUT_MAPPER_TRACKER_CLAZZ =
"phoenix.mapreduce.multi.mapper.tracker.path";
+ // provide control to whether or not handle MR snapshot restore on phoenix
side or handled by caller externally
Review comment:
Also could you please add couple of lines on what does it mean to manage
internally vs externally ?
##########
File path:
phoenix-core/src/it/java/org/apache/phoenix/end2end/TableSnapshotReadsMapReduceIT.java
##########
@@ -61,16 +46,39 @@
import org.apache.phoenix.mapreduce.util.PhoenixMapReduceUtil;
import org.apache.phoenix.schema.types.PDouble;
import org.apache.phoenix.schema.types.PhoenixArray;
+import org.apache.phoenix.thirdparty.com.google.common.collect.Maps;
import org.apache.phoenix.util.ReadOnlyProps;
import org.junit.Assert;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
-
-import org.apache.phoenix.thirdparty.com.google.common.collect.Maps;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.io.IOException;
Review comment:
Import order is changed for all the files. Please revert back to
original format.
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixConfigurationUtil.java
##########
@@ -866,4 +871,18 @@ public static void setTenantId(Configuration
configuration, String tenantId){
configuration.set(MAPREDUCE_TENANT_ID, tenantId);
}
+ public static void setMRSnapshotManagedExternally(Configuration
configuration, Boolean isSnapshotRestoreManagedExternally) {
+ Preconditions.checkNotNull(configuration);
+ Preconditions.checkNotNull(isSnapshotRestoreManagedExternally);
+ configuration.set(MANAGE_MR_SNAPSHOT_RESTORE_EXTERNALLY,
Review comment:
You can use setBoolean so that you don't have to convert boolean to
string.
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/iterate/TableSnapshotResultIterator.java
##########
@@ -18,31 +18,33 @@
package org.apache.phoenix.iterate;
-import java.io.IOException;
-import java.sql.SQLException;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.UUID;
-
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.FileSystem;
import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.HRegionInfo;
import org.apache.hadoop.hbase.PrivateCellUtil;
import org.apache.hadoop.hbase.client.RegionInfo;
import org.apache.hadoop.hbase.client.Scan;
import org.apache.hadoop.hbase.client.TableDescriptor;
import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.hadoop.hbase.shaded.protobuf.generated.SnapshotProtos;
import org.apache.hadoop.hbase.snapshot.RestoreSnapshotHelper;
+import org.apache.hadoop.hbase.snapshot.SnapshotDescriptionUtils;
+import org.apache.hadoop.hbase.snapshot.SnapshotManifest;
import org.apache.hadoop.hbase.util.CommonFSUtils;
-import org.apache.phoenix.compile.ExplainPlanAttributes
- .ExplainPlanAttributesBuilder;
+import
org.apache.phoenix.compile.ExplainPlanAttributes.ExplainPlanAttributesBuilder;
import org.apache.phoenix.mapreduce.util.PhoenixConfigurationUtil;
import org.apache.phoenix.monitoring.ScanMetricsHolder;
import org.apache.phoenix.schema.tuple.Tuple;
import org.apache.phoenix.util.ServerUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.io.IOException;
Review comment:
Same comment about the import order as other comment.
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixConfigurationUtil.java
##########
@@ -51,15 +39,26 @@
import org.apache.phoenix.schema.PTableKey;
import org.apache.phoenix.schema.PTableType;
import org.apache.phoenix.schema.TableNotFoundException;
+import org.apache.phoenix.thirdparty.com.google.common.base.Joiner;
+import org.apache.phoenix.thirdparty.com.google.common.base.Preconditions;
+import org.apache.phoenix.thirdparty.com.google.common.collect.Lists;
import org.apache.phoenix.util.ColumnInfo;
import org.apache.phoenix.util.PhoenixRuntime;
import org.apache.phoenix.util.QueryUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import org.apache.phoenix.thirdparty.com.google.common.base.Joiner;
-import org.apache.phoenix.thirdparty.com.google.common.base.Preconditions;
-import org.apache.phoenix.thirdparty.com.google.common.collect.Lists;
+import java.io.IOException;
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.Base64;
+import java.util.List;
+import java.util.Map;
+import java.util.Properties;
+
+import static org.apache.commons.lang3.StringUtils.isNotEmpty;
+import static
org.apache.phoenix.query.QueryServices.USE_STATS_FOR_PARALLELIZATION;
Review comment:
I think all the static imports go to the top of the import list. Also
all the java.util.* imports should come before org.apache.phoenix.* imports.
Please change that.
##########
File path:
phoenix-core/src/it/java/org/apache/phoenix/end2end/TableSnapshotReadsMapReduceIT.java
##########
@@ -274,6 +293,9 @@ private void configureJob(Job job, String tableName, String
inputQuery, String c
assertFalse("Should only have stored" + result.size() + "rows in the
table for the timestamp!", rs.next());
} finally {
+ if (isSnapshotRestoreDoneExternally) {
+ assertRestoreDirCount(conf, tmpDir.toString(), 1);
Review comment:
Could you assert the restoredircount is more than 1 if
isSnapshotRestoreDoneExternally is set to false ?
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixConfigurationUtil.java
##########
@@ -191,6 +190,12 @@
// provide an absolute path to inject your multi input mapper logic
public static final String MAPREDUCE_MULTI_INPUT_MAPPER_TRACKER_CLAZZ =
"phoenix.mapreduce.multi.mapper.tracker.path";
+ // provide control to whether or not handle MR snapshot restore on phoenix
side or handled by caller externally
Review comment:
Also please change the phoenix conf string from
phoenix.mr.manage.snapshot.restore.externally to
"phoenix.mapreduce.snapshot.restore.externally" to be consistent with other
property names.
----------------------------------------------------------------
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]
