-
Notifications
You must be signed in to change notification settings - Fork 994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHOENIX-5592 MapReduce job to asynchronously delete rows where the VI… #975
Conversation
…EW_TTL has expired
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more files remaining to review.
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/PhoenixMultiViewInputFormat.java
Show resolved
Hide resolved
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/PhoenixMultiViewInputSplit.java
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/PhoenixMultiViewInputSplit.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/org/apache/phoenix/mapreduce/util/DefaultPhoenixMultiViewListProvider.java
Show resolved
Hide resolved
// CASE 2 : BASE_TABLE -> VIEW | ||
PTable parentTable = PhoenixRuntime.getTable(connection, null, | ||
pTable.getParentName().toString()); | ||
if (parentTable.getType() == PTableType.VIEW && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can u add comments here please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
PTable pTable = null; | ||
try { | ||
pTable = PhoenixRuntime.getTable(connection, tenantId, fullTableName); | ||
// we currently only support up to three levels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only upto 3 levels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, we do support multi-levels other than 3, let me remove this.
} | ||
} | ||
if (isQueryMore) { | ||
if (fullTableName == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fullTableName is not being used anywhere after this, so why the check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the fullTableName is checking whether we done the full scan of syscat or not.
If we do have more rows to query, the current fullTableName cannot be null right? If we don't have more rows to scan, the current rs.next will not have any value, and we reset the fullTableName value to null for every iteration.
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixConfigurationUtil.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/ViewInfoWritable.java
Outdated
Show resolved
Hide resolved
…line exceed more than 100 char"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, except for some nits,
Also, can u add some unit tests for most of the framework classes?
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/PhoenixTTLTool.java
Outdated
Show resolved
Hide resolved
public List<InputSplit> generateSplits(List<ViewInfoWritable> views, Configuration configuration) { | ||
int numViewsInSplit = PhoenixConfigurationUtil.getMultiViewSplitSize(configuration); | ||
|
||
if (numViewsInSplit < 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my bad!!
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixMapReduceUtil.java
Outdated
Show resolved
Hide resolved
phoenix-core/src/it/java/org/apache/phoenix/end2end/PhoenixTTLToolIT.java
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
Added more tests |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1
…EW_TTL has expired