-
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… #664
Conversation
…EW_TTL has expired
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/ViewTtlTool.java
Outdated
Show resolved
Hide resolved
85c79e3
to
e1a0ca4
Compare
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/ViewTTLDeleteJobMapper.java
Show resolved
Hide resolved
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/ViewTTLDeleteJobMapper.java
Outdated
Show resolved
Hide resolved
String deleteIfExpiredStatement = "SELECT /*+ NO_INDEX */ count(*) FROM " + value.getViewName(); | ||
deletingExpiredRows(connection, view, Long.valueOf(value.getViewTtl()), | ||
deleteIfExpiredStatement, config); | ||
List<PTable> allIndexesOnView = view.getIndexes(); |
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.
AFAIK, this also gets indexes on the base table. Is it expected?
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.
that's expected. We delete expired rows for the base table, as well as the index table.
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.
@yanxinyi We are only deleting from view indexes right?
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.
yes. that's correct.
} | ||
|
||
@Override public float getProgress() throws IOException, InterruptedException { | ||
return 0; |
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 we evaluate this?
deletingExpiredRows(tenantConnection, value, config); | ||
} | ||
} else { | ||
deletingExpiredRows(connection, value, config); |
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.
I am assuming these are for global views? And have been validated that they do not have any child views. It would be useful to pass that info (global/tenant view) in the value (ViewInfoTracker) so that additional validation can be done here.
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.
We already did check before passing to the mapper. In other words, only leaf view can be processed and deleted
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/ViewTtlTool.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
public String getJobName() { | ||
if (this.jobName == 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.
nit: Can we define a job name format and build that consistently?
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.
all jobs start with prefix ViewTTLTool-
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/ViewTtlTool.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixMultiViewDeletionInputStrategy.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/org/apache/phoenix/mapreduce/util/PhoenixMultiViewDeletionInputStrategy.java
Outdated
Show resolved
Hide resolved
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 nit changes here and there otherwise looking good, will take another pass later.
close this PR since implementation changes. the new PR is here: #762 |
Co-authored-by: Gokcen Iskender <47044859+gokceni@users.noreply.github.com>
…EW_TTL has expired