Add ResourceHandler, FileUtils.map() returning ResourceHandler by leventov · Pull Request #51 · metamx/java-util

leventov · 2016-09-12T16:21:12Z

And improve resource closing in FileSmoosher and SmooshedFileMapper

Better version of #50

…ByteBuffer> and improve resource closing in FileSmoosher and SmooshedFileMapper

drcrallen · 2016-09-12T16:28:23Z

+   * Closes the wrapped resource.
+   */
+  @Override
+  void close();


AutoCloseable.close() allows Exception, is there a need to erase that signature here?

This interface is copied from Druid: io.druid.collections.ResourceHandler. It erases exception, apparently because no present application requires throwing exception, but having it in signature it more noisy on usage side

drcrallen · 2016-09-12T16:36:47Z

This might collide a bit with io.druid.collections.ResourceHolder, if you have any thoughts on how to keep too many "resource holding interfaces" from polluting the namespace please let them be known.

drcrallen · 2016-09-12T16:37:14Z

@leventov please add tests to show the functionality added here is accomplishing what it is intended to accomplish.

drcrallen · 2016-09-12T16:42:16Z

Overall this is a very reasonable, appropriate, and well executed patch. I would like to see some way to limit the quantity of resource handling interfaces.

One concern is that io.druid.collections.ResourceHolder extends Closeable whereas the proposed addition here extends the more restrictive Autocloseable, which means if io.druid.collections.ResourceHolder were to extend the proposed interface, it would also need to extend Closeable if we were to ensure zero interface changes. We can always change the interface though, so that is by no means a strict requirement.

leventov · 2016-09-12T16:58:17Z

It intentionally shadows Druid's ResourceHandler, because 1) ResourceHandler finds it's usage in java-utils source itself 2) Best place for map() is FileUtils, there is no similar class or even package in Druid codebase.

Druid's ResourceHandler is used only internally, so I don't see a big problem if they co-exist with java-utils, or Druid's could be deprecated, or removed.

Another way is adding io.druid.collections.ResourceHandler to java-util directly but it would be strange and module technologies like OSGi or Java 9 modules are not sympathetic to things like this (when multiple modules/project share packages).

leventov · 2016-09-12T17:02:26Z

@leventov please add tests to show the functionality added here is accomplishing what it is intended to accomplish.

The direct reason for this is still ability to run some druid's benchmarks in windows. So it's impossible to write a sensible test for this to run in Linux.

Also some Druid's Files.map()-using code could be simplified, apache/druid#3422 (comment)

leventov · 2016-09-12T17:06:18Z

One concern is that io.druid.collections.ResourceHolder extends Closeable whereas the proposed addition here extends the more restrictive Autocloseable, which means if io.druid.collections.ResourceHolder were to extend the proposed interface, it would also need to extend Closeable if we were to ensure zero interface changes. We can always change the interface though, so that is by no means a strict requirement.

I didn't mean io.druid.collections.ResourceHolder to extend the added ResourceHandler, I see no point but saving literally a couple of lines of code. io.druid.collections.ResourceHolder could be removed if used only internally, or if there is a suspection that some extensions use this interface, we can left it in the codebase until the next major Druid release, as is.

leventov · 2016-09-12T17:07:51Z

Or, the added ResourceHolder could also be made to extend Closeable, it's ok.

drcrallen · 2016-09-12T17:18:37Z

@leventov it's not about saving code, its about saving developer sanity. If someone sees there are two interfaces in place that do essentially the same thing, and have no logical reason they should be different, then that muddles the waters more than it should.

I propose having ResourceHandler from java-util be the interface used in druid instead of ResourceHolder.

leventov · 2016-09-13T16:05:55Z

@gianm could you please review and/or express your opinion as Druid commiter on replacing io.druid.collections.ResourceHandler with com.metamx.common.ResourceHandler?

drcrallen · 2016-09-14T15:45:16Z

As per https://groups.google.com/d/msg/druid-development/SQK4FYxzeXw/i2jx_atBAAAJ looks like the interface is staying here

leventov · 2016-09-14T15:50:38Z

@drcrallen as pulling generic ResourceHandler into java-util aimed to replace Druid's ResourceHandler, what is not going to happen, I can specialize it, i. e. have just MappedByteBufferHandler in java-util. Makes sense?

drcrallen · 2016-09-14T16:16:58Z

+        }
+      }
    }
+    Throwables.propagateIfPossible(thrown);


This changes behavior as it will now completely swallow IOException instead of propagating it, or even logging it.

Is there a reason the exception should be swallowed now?

IOException was not possible here, so I just removed unneeded throws IOException. Only RuntimeException or Error are possible. So Throwables.propagateIfPossible(thrown); is just a way to rethrow, without javac complaining.

drcrallen · 2016-09-14T16:18:30Z

+ * Facilitates using try-with-resources with resources that don't implement {@link AutoCloseable}, e. g.
+ * {@link java.nio.ByteBuffer}s.
+ *
+ * <p>This interface replaces {@code io.druid.collections.ResourceHandler}.


don't forget to remove, you can say its modeled after, but not replaces as per discussion.

leventov · 2016-09-19T10:36:29Z

@cheddar @drcrallen could you please express your opinion on the current state of the PR using the new Github's review system?

drcrallen

Code looks good. needs unit tests for new methods and for showing improved behavior in existing methods

drcrallen · 2016-09-20T18:47:43Z

  public void testSanity() throws Exception
  {
    File baseDir = Files.createTempDir();
+    baseDir.deleteOnExit();


Can you use the TemporaryFolder @Rule for junit for this?

…th-resources; Add FileUtilsTest

leventov · 2016-09-21T17:51:53Z

@drcrallen could you please review latest changes?

drcrallen · 2016-09-21T18:03:09Z

-      File[] files = baseDir.listFiles();
-      Assert.assertEquals(1, files.length);
-      Assert.assertEquals(0, files[0].length());
+    FileSmoosher smoosher = new FileSmoosher(baseDir, 21);


Should this one be handled in a try-with-resources?

Add ResourceHandler, FileUtils.map() returning ResourceHandler<Mapped…

752d741

…ByteBuffer> and improve resource closing in FileSmoosher and SmooshedFileMapper

drcrallen reviewed Sep 12, 2016
View reviewed changes

Add replacement note to ResourceHandler

f6b2d4a

drcrallen reviewed Sep 14, 2016
View reviewed changes

Specialize ResourceHandler as MappedByteBufferHandler

f1315de

drcrallen reviewed Sep 14, 2016
View reviewed changes

drcrallen suggested changes Sep 19, 2016

View reviewed changes

Add SmooshedFileMapperTest.testDeterministicFileUnmapping()

9f18dac

leventov changed the title ~~Add ResourceHandler, FileUtils.map() returning ResourceHandler~~ Add ResourceHandler, FileUtils.map() returning ResourceHandler [WIP] Sep 20, 2016

drcrallen suggested changes Sep 20, 2016

View reviewed changes

Make SmooshedFileMapperTest to use JUnit's TemporaryFolder and try-wi…

c9b2461

…th-resources; Add FileUtilsTest

leventov changed the title ~~Add ResourceHandler, FileUtils.map() returning ResourceHandler [WIP]~~ Add ResourceHandler, FileUtils.map() returning ResourceHandler Sep 20, 2016

Minor fixes

c5c30ed

drcrallen suggested changes Sep 21, 2016

View reviewed changes

Close FileSmoosher in testBehaviorWhenReportedSizesSmall()

3e06ed5

drcrallen approved these changes Sep 21, 2016

View reviewed changes

drcrallen merged commit f02b9ce into metamx:master Sep 21, 2016

drcrallen mentioned this pull request Sep 21, 2016

Update java-util to 0.27.11 apache/druid#3479

Closed

leventov mentioned this pull request Oct 12, 2016

Concise File mapping handling with FileUtils.map() apache/druid#3562

Closed

Conversation

leventov commented Sep 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drcrallen commented Sep 12, 2016

Uh oh!

drcrallen commented Sep 12, 2016

Uh oh!

drcrallen commented Sep 12, 2016

Uh oh!

leventov commented Sep 12, 2016

Uh oh!

leventov commented Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leventov commented Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leventov commented Sep 12, 2016

Uh oh!

drcrallen commented Sep 12, 2016

Uh oh!

leventov commented Sep 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drcrallen commented Sep 14, 2016

Uh oh!

leventov commented Sep 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov commented Sep 19, 2016

Uh oh!

drcrallen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leventov commented Sep 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

leventov commented Sep 12, 2016 •

edited

Loading

leventov commented Sep 12, 2016 •

edited

Loading

leventov commented Sep 13, 2016 •

edited

Loading