File smoosher changes by akashdw · Pull Request #55 · metamx/java-util

akashdw · 2016-09-26T19:08:18Z

This pr is to support FileSmoosher changes proposed in apache/druid#3513 .

FileSmoosher detect when it already has an OutputStream open and redirect newly opened OutputStreams to new files on the file system. When any of the open OutputStream objects are closed, they will also check to see if any of the other files have been closed in the meantime. If there are OutputStreams that have been closed, they will be copied on to the main smoosh file and the extra underlying file will be cleaned up. This has the downside of introducing an extra copy to these files, but it given that the strategy will only be used when absolutely necessary, this shouldn’t result in any noticeable performance degradation during indexing.

…e_smoosher_changes

cheddar · 2016-09-26T19:33:14Z

  {
+    if(writerCurrentlyInUse)
+    {
+      return delegateSmooshedWriter (name , size);


weird spacing

cheddar · 2016-09-26T19:33:30Z

  private final List<File> outFiles = Lists.newArrayList();
  private final Map<String, Metadata> internalFiles = Maps.newTreeMap();
+  private List<File> files = Lists.newArrayList();
+  private List<File> createFiles = Lists.newArrayList();


Should this be past-tense?

Actually, how about a rename to filesInProcess?

cheddar · 2016-09-26T19:34:17Z

          throw new IOException(
              String.format("Expected [%,d] bytes, only saw [%,d], potential corruption?", size, bytesWritten)
-          );
+              );


This whitespace appears to be off...

cheddar · 2016-09-26T19:35:28Z

+              );
+        }
+
+        if(!writerCurrentlyInUse)


Given that you just set writerCurrentlyInUse to false and that this object is not attempting to be thread-safe, I don't think this can ever be false.

cheddar · 2016-09-26T19:42:52Z

+        return addToOffset(Ints.checkedCast(java.nio.file.Files.copy(in, tmpFile.toPath())));
+      }
+
+      public int addToOffset(int numBytesWritten)


This method has a really weird contract.

Why is it public?

Why does it return its incoming argument without any adjustment?

The name of the method makes it seem like this would be returning the total offset instead of the size. The methods in the non-delegate writer all do this call first with the name "verify" which makes it clear that the intent of the method is to do some state checks...

Agree, It shouldn't be. Made the code changes to make it more meaningful.

cheddar · 2016-09-26T19:44:16Z


  private final List<File> outFiles = Lists.newArrayList();
  private final Map<String, Metadata> internalFiles = Maps.newTreeMap();
+  private List<File> files = Lists.newArrayList();


How about renaming this to completedFiles.

cheddar · 2016-09-26T19:45:34Z

+      {
+        open = false;
+        out.close();
+        files.add(tmpFile);


Why wouldn't we remove the file from the createFiles or the filesInProcess here?

cheddar · 2016-09-26T19:47:22Z

-      }
-    }
+  @Test
+  public void testWhenFirstWriterClosedInTheLast() throws Exception


Does "InTheLast" mean "AtTheEnd" or something else?

leventov · 2016-09-30T19:09:26Z

  {
+    if(writerCurrentlyInUse)
+    {
+      return delegateSmooshedWriter(name , size);


space before ,

leventov · 2016-09-30T19:11:33Z


  public SmooshedWriter addWithSmooshedWriter(final String name, final long size) throws IOException
  {
+    if(writerCurrentlyInUse)


Logic should go after argument check (if block below).

Here should be a space after if

leventov · 2016-09-30T19:14:26Z

+    }
+  }
+
+  private SmooshedWriter delegateSmooshedWriter(final String name,final long size) throws IOException


space after ,

leventov · 2016-09-30T19:18:37Z

+        completedFiles.add(tmpFile);
+        filesInProcess.remove(tmpFile);
+
+        if(!writerCurrentlyInUse ) {


leventov · 2016-09-30T19:20:02Z

+  {	
+    final File tmpFile = new File(baseDir, name);
+    filesInProcess.add(tmpFile);
+    return new SmooshedWriter () {


no space before (), { on the new line

logarithm · 2016-09-30T19:46:18Z


  public SmooshedWriter addWithSmooshedWriter(final String name, final long size) throws IOException
  {
+    if(writerCurrentlyInUse)


Here should be a space after if

logarithm · 2016-09-30T19:46:46Z

+    //get processed elements from the stack and write.
+    List<File> fileToProcess = new ArrayList<>(completedFiles);
+    completedFiles = Lists.newArrayList();
+    for(File file: fileToProcess)


Here should be a space after for

logarithm · 2016-09-30T19:47:46Z

+    //book keeping checks on created file.
+    if(!completedFiles.isEmpty())
+    {
+      for(File file: completedFiles)


Here should be a space after for

logarithm · 2016-09-30T19:47:52Z

  public void close() throws IOException
  {
+    //book keeping checks on created file.
+    if(!completedFiles.isEmpty())


Here should be a space after if

akashdw · 2016-10-04T19:11:51Z

@logarithm @leventov @cheddar Thanks for reviewing the PR. I addressed all the comments and updated the pr. My Eclipse auto-formatter was turned off which caused the formatting issues.

cheddar · 2016-10-04T19:56:42Z

👍

leventov · 2016-10-04T19:59:24Z

Please don't merge until the end of this week, I want to review this.

leventov

Rebase onto the current master.
There are still many code style problems. Please get this file: https://github.com/druid-io/druid/blob/master/druid_intellij_formatting.xml, import it into idea, and run "Reformat Code". Or, if you use Eclipse: https://github.com/druid-io/druid/blob/master/eclipse_formatting.xml
I honestly tried to understand this code for some time but failed. I don't understand why we need two new collections of files, why they replace each other, the meaning of methods, etc. I cannot approve the PR in this state because it's too complex to understand (hence to support). You should either add extensive comments on the new fields and methods, explaining what is going on, or (better) find a simpler way to accomplish what you want.

akashdw · 2016-10-07T19:52:17Z

@leventov Thanks for looking into this PR, I'm using https://github.com/druid-io/druid/blob/master/eclipse_formatting.xml to format code. I'll update my local settings if that setting got updated.
I updated the code and addressed all the formatting comments, not sure why github is not showing the updated code in comment section, however files changes section is showing formatted code.

akashdw · 2016-10-08T00:27:04Z

@leventov Updated the PR and added comments to explain code logic.

leventov

Ok, I see now. Thanks for the changes. I've also read original issue in Druid.

I think this whole thing should better be implemented using composition over FileSmoosher, rather than adding new logic to this class.

Please create a class like

class MultiSmoosher { // choose a better name please
  private final FileSmoosher delegate;
  private List<File> completedFiles;
  private List<File> filesInProcess;
}

And move the logic, which you have added to FileSmoosher in this PR, to this class.

I think this wrapper class could have specialized methods for writing to the delegate fileSmoosher directly and to temp files, because you know what do you need in the call sites in Druid. So the tricky writerCurrentlyInUse logic is not needed, I believe.

This class should be added to Druid codebase directly, not java-util.

leventov · 2016-10-10T18:17:38Z


  private final List<File> outFiles = Lists.newArrayList();
  private final Map<String, Metadata> internalFiles = Maps.newTreeMap();
+  // list of files completed writing content using delegated smooshedWriter.


I think it is better to write such comments as Javadoc /** ... */, then on automatic refactorings and reformattings IDEs will move comments along with fields. Javadoc comments are allowed for private fields.

leventov · 2016-10-10T18:20:23Z

+  private void mergeWithSmoosher() throws IOException
+  {
+    // Get processed elements from the stack and write.
+    List<File> fileToProcess = new ArrayList<>(completedFiles);


Maybe it can be just iteration over completedFiles (currently lines 251-256), then completedFiles.clear().

leventov · 2016-10-10T18:21:42Z

+    {
+      for (File file: completedFiles)
+      {
+        file.delete();


Maybe merge completedFiles here using mergeWithSmoosher(), rather than silently delete?

Entry into the if statement bounding these for loops causes an exception to be thrown, the deletion is to clean up state before that exception is thrown.

leventov · 2016-10-10T18:24:02Z

+      {
+        file.delete();
+      }
+      for (File file: filesInProcess)


Maybe throw some exception here, if filesToProcess is not empty? Because presence of files in filesInProcess means somebody opened a smoosherWriter but didn't close it before closing FileSmoosher. It seems like a bug.

There is an exception thrown just as soon as this for loop exits for exactly that reason. It's a bug if any files haven't yet been merged by the time the larger Smoosher gets closed (each of the individual Writers should have been closed and those should have caused everything to get merged).

cheddar · 2016-10-10T20:46:44Z

@leventov the idea to do it with composition is definitely interesting. It poses a few problems as well because the only thing that currently has access to the SmooshedWriter to know if it is in use or not is the FileSmoosher. So, the wrapping class would essentially also have to wrap the SmooshedWriter that is returned.

Also, there is a bit of state that the FileSmoosher necessarily owns, namely the directory that files are being created in. Switching to composition would force this state to somehow be shared with the wrapping class (or the wrapping class would have to have yet another directory available to it for the storing of files).

I think that this code belongs inside FileSmoosher and going through the steps to make it work with composition will be just a bunch of code baggage without much benefit.

Can you elaborate on why you are afraid to change FileSmoosher?

leventov · 2016-10-10T23:27:58Z

@cheddar

It poses a few problems as well because the only thing that currently has access to the SmooshedWriter to know if it is in use or not is the FileSmoosher. So, the wrapping class would essentially also have to wrap the SmooshedWriter that is returned.

From the description of apache/druid#3513 I understood that @akashdw is going to pass FileSmoosher somewhere while SmoosherWriter is open. AFAIU it's not recursion, i. e. FileSmoosher is passed to some different place. A wrapper class could essentially have delegateSmooshedWriter() method public (however I would name it differently, somehow involving adjective "delayed"), and call this method from that other place. Don't need to track the FileSmoosher itself. I written this already in other words:

I think this wrapper class could have specialized methods for writing to the delegate fileSmoosher directly and to temp files, because you know what do you need in the call sites in Druid. So the tricky writerCurrentlyInUse logic is not needed, I believe.

Also, there is a bit of state that the FileSmoosher necessarily owns, namely the directory that files are being created in. Switching to composition would force this state to somehow be shared with the wrapping class (or the wrapping class would have to have yet another directory available to it for the storing of files).

The only bit of state that new logic accesses is baseDir, to create temp files. However temp files don't need (and actually shouldn't) be created in the smoosher directory itself. They should better be created via Java standard, or Guava's utility methods for creating temp files, which end up placing it in java.io.temp dir or /tmp, which is likely to be mounted to tmpfs, while the smoosher directory is less likely to be there (?)

So, I don't see the new logic is tightly coupled with the FileSmoosher state. Also a couple of weeks ago you mentioned that it's better to add code to druid than java-util. This is also the case, I suppose.

leventov · 2016-10-10T23:35:27Z

@cheddar Also, I think it's quite likely that @akashdw's proposed design of adding functionality to Druid which he wants to add will be changed sooner or later. Maybe even as soon as on the PR review stage. So that this change to FileSmoosher is no longer needed, for whatever reason. But we will need to carry it because java-util is a library. While logic from Druid core could be removed freely, because it is not a library. Also don't forget about boilerplate of having separate PRs and release cycle of java-util and Druid...

cheddar · 2016-10-11T00:02:11Z

Ok, we will just move this to be part of the larger PR in druid.

cheddar · 2016-10-11T16:14:08Z

Btw,

They should better be created via Java standard, or Guava's utility methods for creating temp files, which end up placing it in java.io.temp dir or /tmp, which is likely to be mounted to tmpfs, while the smoosher directory is less likely to be there (?)

We cannot and should not do that. By writing into random temporary locations, you are placing extra data in arbitrary places that the users do not have strong control over. The directories used inside of Druid for storing these things have a very specific lifecycle and get cleaned up according to that lifecycle. Reusing those directories is important and using tmp locations is a non-starter (and often leads to orphaned files and users not understanding why things have filled up).

leventov · 2016-10-11T16:52:25Z

@cheddar thanks, this makes sense. So, smooshed file directories are temporal as a whole? And could be located in tmpfs?

So if "MultiSmoosher" needs to access FileSmoosher's baseDir, it could be passed along with FileSmoosher in constructor, or getBaseDir() accessor could be added to FileSmoosher.

drcrallen

On a softer note, I suggest checking the result of delete() and at least log a warning if you are unable to delete a file.

drcrallen · 2016-11-03T16:02:33Z

+ * various "chunk" files will be varying sizes and it is not possible to add a
+ * file of size greater than Integer.MAX_VALUE.
+ * <p/>
+ * This class is not thread safe but allows writing multiple files even if main


Please change to a straight forward declaration like This class is no thread safe on its own line.

You can describe the handling of multiple files in another paragraph but such description should in no way mangled with explanations of thread safett.

drcrallen · 2016-11-04T04:29:56Z

+        if (i==10)
+        {
+          writer.write(ByteBuffer.wrap(Ints.toByteArray(19)));
+          CloseQuietly.close(writer);


why quietly?

drcrallen · 2016-11-04T04:30:10Z

+        smoosher.add(String.format("%d", i), tmpFile);
+        tmpFile.delete();
+      }
+      CloseQuietly.close(writer);


why quietly?

drcrallen · 2016-11-04T04:30:58Z

+      }
+    }
+  }
+}


Missing newline

from metamx/java-util#55

drcrallen · 2016-11-09T18:42:19Z

Thanks!

* FileSmoosher requested changes from metamx/java-util#55 * Addressed code review requested changes.

akashdw and others added 2 commits August 12, 2016 09:27

File smoosher changes.

fa85969

Merge branch 'master' of https://github.com/metamx/java-util into fil…

4af8c31

…e_smoosher_changes

cheddar suggested changes Sep 26, 2016

View reviewed changes

dwivedi added 3 commits September 30, 2016 11:02

addressed comment

43aec86

formatting.

faf2aa4

formatting.

034a374

leventov suggested changes Sep 30, 2016

View reviewed changes

logarithm suggested changes Sep 30, 2016

View reviewed changes

addressed comment and formatting.

b060cd6

leventov mentioned this pull request Oct 4, 2016

Checkstyle apache/druid#3533

Closed

leventov suggested changes Oct 7, 2016

View reviewed changes

akashdw added 2 commits October 7, 2016 16:56

Merge branch 'master' into file_smoosher_changes

a6425c0

Fixed comments.

72d6f21

leventov reviewed Oct 10, 2016

View reviewed changes

cheddar mentioned this pull request Nov 1, 2016

FileSmoosher changes snuck in with java-util merge apache/druid#3640

Closed

drcrallen suggested changes Nov 4, 2016

View reviewed changes

akashdw added a commit to akashdw/druid that referenced this pull request Nov 9, 2016

FileSmoosher requested changes

96a98f1

from metamx/java-util#55

akashdw mentioned this pull request Nov 9, 2016

FileSmoosher requested changes apache/druid#3673

Merged

drcrallen approved these changes Nov 9, 2016

View reviewed changes

logarithm approved these changes Nov 9, 2016

View reviewed changes

fjy pushed a commit to apache/druid that referenced this pull request Nov 10, 2016

FileSmoosher requested changes (#3673)

1acc816

* FileSmoosher requested changes from metamx/java-util#55 * Addressed code review requested changes.

leventov closed this Nov 10, 2016

fundead pushed a commit to fundead/druid that referenced this pull request Dec 7, 2016

FileSmoosher requested changes (apache#3673)

45cf8a1

* FileSmoosher requested changes from metamx/java-util#55 * Addressed code review requested changes.

dgolitsyn pushed a commit to metamx/druid that referenced this pull request Feb 14, 2017

FileSmoosher requested changes (apache#3673)

7de6a57

* FileSmoosher requested changes from metamx/java-util#55 * Addressed code review requested changes.

+                    }
+                  }
+                }
+              }

                
                    No newline at end of file

Conversation

akashdw commented Sep 26, 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!

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!

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!

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!

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

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!

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!

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

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!

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!

akashdw commented Oct 4, 2016

Uh oh!

cheddar commented Oct 4, 2016

Uh oh!

leventov commented Oct 4, 2016

Uh oh!

leventov left a comment

Choose a reason for hiding this comment

Uh oh!

akashdw commented Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akashdw commented Oct 8, 2016

logarithm Sep 30, 2016 •

edited

Loading

logarithm Sep 30, 2016 •

edited

Loading

akashdw commented Oct 7, 2016 •

edited

Loading

leventov Oct 10, 2016 •

edited

Loading

leventov Oct 10, 2016 •

edited

Loading

leventov commented Oct 10, 2016 •

edited

Loading

leventov commented Oct 10, 2016 •

edited

Loading