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
ACCUMULO-3602 BatchScanner optimization for AccumuloInputFormat #25
Conversation
also: killing dead code for “iterators”
Accumulo-Pull-Requests #21 FAILURE |
Accumulo-Pull-Requests #22 SUCCESS |
…MULO-3602 Conflicts: core/src/main/java/org/apache/accumulo/core/client/mapred/AbstractInputFormat.java core/src/main/java/org/apache/accumulo/core/client/mapreduce/AbstractInputFormat.java core/src/main/java/org/apache/accumulo/core/client/mapreduce/RangeInputSplit.java
Accumulo-Pull-Requests #23 FAILURE |
Ack, I'm sorry, @echeipesh, I just broke you. |
No worries, I was just looking through this, I see what happened. |
Accumulo-Pull-Requests #24 SUCCESS |
scanner.setRange(rangeSplit.getRange()); | ||
|
||
// do this last after setting all scanner options | ||
scannerIterator = scanner.iterator(); |
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.
Duplicated call to scanner.iterator()
. Looks like you do it down below too?
Summary of changes since last burst of comments:
|
Accumulo-Pull-Requests #43 SUCCESS |
|
||
setupIterators(iterators, scanner); | ||
protected List<IteratorSetting> jobIterators(JobConf job, String tableName) { | ||
return getIterators(job); |
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'm not seeing where this is implemented. Am I just blind?
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.
n/m, pulled it into Eclipse and realized it's just me :)
Ok, two things I think need to be fixed. Everything else you fixed looks good!
I pulled down the PR and ran it through japi which is how I noticed the first point. Otherwise it looks good from a compatibility sense for 1.6->1.7. Seeing it in Eclipse brought up some checkstyle warnings still (unused imports). If you want to clean those up, that'd be great; otherwise, whomever merges can probably kill them all at once. |
I think this looks good. I am finished looking at it. I also think adding the |
Accumulo-Pull-Requests #47 FAILURE |
Accumulo-Pull-Requests #48 FAILURE |
👍 from me |
+1 |
Accumulo-Pull-Requests #49 FAILURE |
Accumulo-Pull-Requests #51 FAILURE |
Accumulo-Pull-Requests #52 SUCCESS |
Hot diggity. I'll see if I can bring this down, run some of the UTs/ITs, squash the commits, and push it up. Thanks so much for your hard work, @echeipesh!! |
Actually, compat is still broken with that setupIterators method. Do you want to fix that @echeipesh or should I just do when I apply the rest of your changes? |
Accumulo-Pull-Requests #54 SUCCESS |
|
And, if you can @echeipesh, can you please close this? Because I squashed and didn't include the magic github markdown, I can't close the PR myself. |
@joshelser, awesome! Thank you for all the help guiding this along. |
No description provided.