Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6273: Add support to handle MR Snapshot restore externally - #1079
Conversation
stoty
commented
Jan 8, 2021
💔 -1 overall
This message was automatically generated. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
stoty
commented
Jan 15, 2021
💔 -1 overall
This message was automatically generated. |
sakshamgangwar
commented
Jan 16, 2021
@gjacoby126 Can you review again? As discussed offline, I have added configuration to handle whether we want to take care of snapshot restore within phoenix (this is being set as default) or we want to do it externally. |
stoty
commented
Jan 16, 2021
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 16, 2021
💔 -1 overall
This message was automatically generated. |
| for (RegionInfo restoredRegion : restoredRegions) { | ||
| if (isValidRegion(restoredRegion)) { | ||
| this.regions.add(restoredRegion); | ||
| if (PhoenixConfigurationUtil.getMRSnapshotManagedInternally(configuration)) { |
There was a problem hiding this 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?
There was a problem hiding this comment.
@bharathv doing the restore per MR job can be handled at one place but I was not able to get any spot to delete/cleanup the restore right after the job is done on phoenix side. On the caller side, I am able to do the cleanup, so in order to handle it in a clean way: I added this configuration to explicitly say: whether or not restore needs to be managed internally or externally. If internally I am keeping the original flow (with issue) as it is. Externally: restore + delete happens on the MR job caller side.
There was a problem hiding this comment.
IMO the original flow is wrong and can lead to pretty bad outcomes (like a file blow up), so we should get rid of restoring per task. If you still want to use this SnapshotManagedInternally() configuration, I'd suggest move it to once at setup and not once per scan.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
sakshamgangwar
commented
Jan 21, 2021
@bharathv@gjacoby126 Can you review it one more time? Made changes based on review comments. |
shahrs87
commented
Jan 21, 2021
If you change the title to all caps PHOENIX (for e.g.: PHOENIX-6273: Add support to handle MR Snapshot restore externally), it will link the PR to jira so that anyone can go directly from jira to this PR. |
| // 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 |
There was a problem hiding this comment.
The naming convention for all other conf property starts with MAPREDUCE. Do you mind changing the property name to something like: "MAPREDUCE_SNAPSHOT_RESTORE_EXTERNAL" ? and prefix with DEFAULT for default value ?
There was a problem hiding this 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.
| public static boolean isMRSnapshotManagedExternally(final Configuration configuration) { | ||
| Preconditions.checkNotNull(configuration); | ||
| boolean isSnapshotRestoreManagedInternally = |
There was a problem hiding this comment.
Typo in the variable name: isSnapshotRestoreManagedInternally. Should it be isSnapshotRestoreManagedExternally ?
| // 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 |
There was a problem hiding this comment.
Also could you please add couple of lines on what does it mean to manage internally vs externally ?
| import java.util.Properties; | ||
| import static org.apache.commons.lang3.StringUtils.isNotEmpty; | ||
| import static org.apache.phoenix.query.QueryServices.USE_STATS_FOR_PARALLELIZATION; |
There was a problem hiding this 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.
| public static void setMRSnapshotManagedExternally(Configuration configuration, Boolean isSnapshotRestoreManagedExternally) { | ||
| Preconditions.checkNotNull(configuration); | ||
| Preconditions.checkNotNull(isSnapshotRestoreManagedExternally); | ||
| configuration.set(MANAGE_MR_SNAPSHOT_RESTORE_EXTERNALLY, |
There was a problem hiding this comment.
You can use setBoolean so that you don't have to convert boolean to string.
| // 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 |
There was a problem hiding this 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.
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import java.io.IOException; |
There was a problem hiding this comment.
Same comment about the import order as other comment.
| assertFalse("Should only have stored" + result.size() + "rows in the table for the timestamp!", rs.next()); | ||
| } finally { | ||
| if (isSnapshotRestoreDoneExternally) { | ||
| assertRestoreDirCount(conf, tmpDir.toString(), 1); |
There was a problem hiding this comment.
Could you assert the restoredircount is more than 1 if isSnapshotRestoreDoneExternally is set to false ?
There was a problem hiding this comment.
@shahrs87 There were two levels of subdirectories getting created for snapshot restore on every scan:
I have removed those in the original flow so now the directory gets cleaned up every single scan and gets created again for the next scan with the same directory structure.
I can assert here no existence of the restore directory in the original flow.
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import java.io.IOException; |
There was a problem hiding this comment.
Import order is changed for all the files. Please revert back to original format.
stoty
commented
Jan 21, 2021
💔 -1 overall
This message was automatically generated. |
sakshamgangwar
commented
Jan 21, 2021
@shahrs87@gjacoby126 Addressed comments on this PR, can you review again? Thanks! |
| this.scanIterator = UNINITIALIZED_SCANNER; | ||
| this.restoreDir = new Path(configuration.get(PhoenixConfigurationUtil.RESTORE_DIR_KEY), | ||
| UUID.randomUUID().toString()); | ||
| this.restoreDir = new Path(configuration.get(PhoenixConfigurationUtil.RESTORE_DIR_KEY)); |
There was a problem hiding this comment.
Will it change any semantics if we are not adding uuid to the restoreDir path when isMRSnapshotManagedExternally is set to false ?
There was a problem hiding this comment.
@shahrs87 I don't think so. I believe the original flow was faulty in itself and for every scan, we were creating a subdirectory, not instead of that, we have the same subdirectory for restore and every scan restore happens there and clean up also happens per scan. So there should not be any issue. We want to get rid of this structure because when we do an external restore or external cleanup, we need to provide exact restore directory for reading it.
There was a problem hiding this comment.
@sakshamgangwar - Remember that scans are taking place in parallel as multiple mappers may be running at the same time. Until we fix the automation to only create once in a future JIRA, don't we need to keep the restored files in separate directories so tasks won't step on each other?
There was a problem hiding this comment.
@gjacoby126I missed that part, thanks for catching that. I will guard the directories with the same config as well.
There was a problem hiding this comment.
@gjacoby126 Can you review again? I have kept old behavior as it is, as well as in the assertion for restore directory I am asserting whether or not snapshot directory exists (in case of externally cleaned up) or not (cleaned up per scan).
bharathv
left a comment
There was a problem hiding this comment.
meat of the patch lgtm, haven't looked at the tests but looks like other reviewers are taking a deeper look. +1.
stoty
commented
Jan 21, 2021
💔 -1 overall
This message was automatically generated. |
| if (PhoenixConfigurationUtil.isMRSnapshotManagedExternally(configuration)) { | ||
| this.restoreDir = new Path(configuration.get(PhoenixConfigurationUtil.RESTORE_DIR_KEY)); | ||
| } else { | ||
| this.restoreDir = new Path(configuration.get(PhoenixConfigurationUtil.RESTORE_DIR_KEY), |
There was a problem hiding this comment.
@sakshamgangwar why do we do the UUID in both the PhoenixMapReduceUtil and TableSnapshotResultIterator? Shouldn't it be one or the other? (If so, putting here in TableSnapshotResultIterator seems clearer to me.)
There was a problem hiding this comment.
@gjacoby126 that existed before this change as well, this is not a necessary code, I believe at job level in PhoenixMapReduceUtil we update the configuration restoreDir to a new path so as to make sure that even if restoreDir configuration sent from the caller is same for multiple jobs, it should be able to handle it internally by adding a new sub-directory (which we are also doing at scan level too :) ). Whereas in externally managed now we take full responsibility for the restoreDir creation/usage/deletion.
gjacoby126
left a comment
There was a problem hiding this comment.
+1, assuming tests pass. Thanks @sakshamgangwar .
stoty
commented
Jan 22, 2021
💔 -1 overall
This message was automatically generated. |
stoty
commented
Jan 22, 2021
💔 -1 overall
This message was automatically generated. |
gjacoby126
commented
Jan 22, 2021
@shahrs87 , when you get a minute could you please take another look and approve or request further changes? |
sakshamgangwar
commented
Jan 22, 2021
@gjacoby126@shahrs87 I am observing only one test failure which is not related to my changes: |
https://issues.apache.org/jira/browse/PHOENIX-6273