PHOENIX-5592 MapReduce job to asynchronously delete rows where the VIEW_TTL has expired #762
Conversation
yanxinyi
changed the title
…EW_TTL has expired
… end users can have customized metrics based on the STATE
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/PhoenixMultiViewInputFormat.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/PhoenixMultiViewReader.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/ViewTTLDeleteJobMapper.java
Outdated
Show resolved
Hide resolved
| initMultiViewJobStatusTracker(config); | ||
| } | ||
|
|
||
| LOGGER.debug(String.format("Deleting from view %s, TenantID %s, and TTL value: %d", |
There was a problem hiding this comment.
Should we log at debug level only if debug level enabled?
There was a problem hiding this comment.
without the config of debug-level enabled, it will omit the logging for the debug level logging.
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/ViewTTLDeleteJobMapper.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/DefaultMultiViewSplitStrategy.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/MultiViewSplitStrategy.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixViewTtlUtil.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/org/apache/phoenix/mapreduce/util/DefaultPhoenixMultiViewListProvider.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/DefaultMultiViewSplitStrategy.java
Show resolved
Hide resolved
| @@ -24,6 +24,25 @@ | |||
| import java.io.IOException; | |||
|
|
|||
| public interface ViewInfoWritable extends Writable { | |||
| public enum ViewInfoJobState { | |||
There was a problem hiding this comment.
Good job on reafctoring it out to be more generic.
| @@ -24,24 +24,6 @@ | |||
| import java.io.IOException; | |||
|
|
|||
| public class ViewInfoTracker implements ViewInfoWritable { | |||
There was a problem hiding this comment.
nit: Good job on refactoring out it to an interface. Now that you have refactored it out it can be called ViewTTLInfoWritable? Since it contains TTL related info
There was a problem hiding this comment.
Overall it looks LGTM to me, apart from a few nits.
Can you get @ChinmaySKulkarni to also look at it.
|
@xinyi Another suggestion I have is whether u want to split this into 2 PRs?
|
There was a problem hiding this comment.
Overall looks good @yanxinyi. Great job! I have provided some comments. Overall, please check that we create Phoenix/HBase connections and Phoenix statements via some sort of try-with-resources to avoid any resource leaks.
|
|
||
| private final int ONE = 1; | ||
| private final int ZERO = 0; | ||
| private final int NUMBER_OF_UPSERT_ROWS = 202; |
There was a problem hiding this comment.
Just curious, any reason we choose 202?
There was a problem hiding this comment.
since the default split size is 100, it will have an uneven split in this case.
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/ViewTTLDeleteJobMapper.java
Show resolved
Hide resolved
...ore/src/main/java/org/apache/phoenix/mapreduce/util/DefaultPhoenixMultiViewListProvider.java
Show resolved
Hide resolved
| this.multiViewJobStatusTracker = (MultiViewJobStatusTracker) defaultViewDeletionTrackerClass.newInstance(); | ||
| } catch (Exception e) { | ||
| LOGGER.info("exception " + e.getMessage()); | ||
| LOGGER.info("stack trace" + e.getStackTrace().toString()); |
There was a problem hiding this comment.
Why are we swallowing the exception here? Can it be rethrown? Also, in case we DO want to swallow the exception and just log it, this should be error level.
|
Already merged. |
…EW_TTL has expired