Skip to content

Comments

ACCUMULO-4195 Added generalized configuration objects for RFile interaction#95

Closed
ShawnWalker wants to merge 3 commits intoapache:masterfrom
ShawnWalker:ACCUMULO-4195
Closed

ACCUMULO-4195 Added generalized configuration objects for RFile interaction#95
ShawnWalker wants to merge 3 commits intoapache:masterfrom
ShawnWalker:ACCUMULO-4195

Conversation

@ShawnWalker
Copy link
Contributor

Implemented a builder/fluent style of syntax, where each operation creates an operation object with both some methods for setting
parameters and an execute() method to actually invoke the operation.

I'm not really sure if this is in line with the improvement the reporter suggests. It does however address the only goal I can identify with that request: it should be possible to extend the interface FileOperations in a manner such as was done for ACCUMULO-4187 without requiring significant changes to unrelated code.

Also, the automatic formatter hates me, and completely undid most of my eye-pleasing formatting.

BlockCache indexCache = input.getIndexCache();
BlockCache dataCache = input.getDataCache();
if (!input.getTableConfiguration().getBoolean(Property.TABLE_INDEXCACHE_ENABLED)) {
indexCache = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seem like this is setting a local var to null.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevemind. I thought this had no impact, but I was looking at the subtracted lines of the diff. Now I see that it does have an impact.

@joshelser
Copy link
Member

@keith-turner how, if at all, does this play into your RFile API work? Have you and Shawn talked about this at all offline?

@joshelser
Copy link
Member

Overall, I think this is a nice improvement @ShawnWalker. I really appreciate the quick turn-around on my request! I'll try to run what you have now through some tests locally.

@ShawnWalker
Copy link
Contributor Author

I changed the method names to look more builder-ish as @joshelser suggested. Wasn't quite sure how the change the verbiage getFileSize(...), so I left it alone.

I also dovetailled NeedsFile into NeedsTableConfiguration, cutting down the signature of the factory methods somewhat.

…action.

Implemented a builder/fluent style of syntax, where each operation creates an operation object with both some methods for setting
parameters and an execute() method to actually invoke the operation.
…so significantly increased type-fu so to present a tighter interface.
@joshelser
Copy link
Member

Nice, I think these changes look good.

mvn verify -Psunny also passed:

[INFO] Reactor Summary:
[INFO]
[INFO] Apache Accumulo Project ............................ SUCCESS [  5.504 s]
[INFO] Apache Accumulo Fate ............................... SUCCESS [ 45.591 s]
[INFO] Apache Accumulo Start .............................. SUCCESS [01:08 min]
[INFO] Apache Accumulo Core ............................... SUCCESS [03:59 min]
[INFO] Apache Accumulo Server Base ........................ SUCCESS [02:12 min]
[INFO] Apache Accumulo Tracer Server ...................... SUCCESS [ 22.325 s]
[INFO] Apache Accumulo Shell .............................. SUCCESS [ 31.890 s]
[INFO] Apache Accumulo Simple Examples .................... SUCCESS [ 19.693 s]
[INFO] Apache Accumulo GC Server .......................... SUCCESS [ 16.644 s]
[INFO] Apache Accumulo Master Server ...................... SUCCESS [ 27.389 s]
[INFO] Apache Accumulo Monitor Server ..................... SUCCESS [ 21.547 s]
[INFO] Apache Accumulo Tablet Server ...................... SUCCESS [ 58.315 s]
[INFO] Apache Accumulo MiniCluster ........................ SUCCESS [01:23 min]
[INFO] Apache Accumulo Native Libraries ................... SUCCESS [  4.775 s]
[INFO] Apache Accumulo Proxy .............................. SUCCESS [01:05 min]
[INFO] Apache Accumulo Trace .............................. SUCCESS [  6.256 s]
[INFO] Apache Accumulo Iterator Test Harness .............. SUCCESS [  8.341 s]
[INFO] Apache Accumulo Testing ............................ SUCCESS [13:16 min]
[INFO] Apache Accumulo .................................... SUCCESS [  8.178 s]
[INFO] Apache Accumulo Documentation ...................... SUCCESS [  0.415 s]
[INFO] Apache Accumulo Maven Plugin ....................... SUCCESS [ 47.383 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 28:31 min
[INFO] Finished at: 2016-04-27T13:20:53-04:00
[INFO] Final Memory: 94M/617M
[INFO] ------------------------------------------------------------------------

Any further comments @keith-turner before I merge this in?


if (out == null) {
out = FileOperations.getInstance().openWriter(file.toString(), file.getFileSystem(conf), conf, null, acuConf);
out = FileOperations.getInstance().newWriterBuilder().forFile(file.toString(), file.getFileSystem(conf), conf).withTableConfiguration(acuConf)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't look too closely, but it struck me this could be cleaned up with another constructor that took a Path and Configuration so it would read
out = FileOperations.getInstance().newWriterBuilder().forFile(file, conf).withTableConfiguration(acuConf)
Fine as is though

@keith-turner
Copy link
Contributor

Any further comments @keith-turner before I merge this in?

Nope. Just took a last look at it. Looks good.

@joshelser
Copy link
Member

Nope. Just took a last look at it. Looks good.

Great. Thanks bud. I will merge this in today.

@asfgit asfgit closed this in 4a79d58 Apr 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants