Skip to content

Unmap files to add eagerly in FileSmoother #50

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.

Sign up for GitHub

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

Closed
leventov wants to merge 1 commit into from
Closed

Unmap files to add eagerly in FileSmoother #50

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/main/java/com/metamx/common/io/smoosh/FileSmoosher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.common.io.Closeables;
import com.google.common.io.Files;
import com.google.common.primitives.Ints;
import com.metamx.common.ByteBufferUtils;
import com.metamx.common.IAE;
import com.metamx.common.ISE;

Expand All @@ -40,6 +41,7 @@
import java.io.OutputStreamWriter;
import java.io.Writer;
import java.nio.ByteBuffer;
import java.nio.MappedByteBuffer;
import java.nio.channels.Channels;
import java.nio.channels.WritableByteChannel;
import java.util.Arrays;
Expand All @@ -53,6 +55,7 @@
* <p/>
* It does not split input files among separate output files, instead the various "chunk" files will
* be varying sizes and it is not possible to add a file of size greater than Integer.MAX_VALUE
* TODO(leventov): consider making this class final
Copy link
Copy Markdown
Contributor

@gianm gianm Sep 7, 2016

Choose a reason for hiding this comment

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

I know this isn't Druid, but generally in Druid we try to minimize TODOs in the code, in favor of Github issues or in favor of just actually doing the thing if it's really important.

Copy link
Copy Markdown
Contributor Author

@leventov leventov Sep 7, 2016

Choose a reason for hiding this comment

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

@gianm thanks, will know.

*/
public class FileSmoosher implements Closeable
{
Expand Down Expand Up @@ -105,12 +108,17 @@ public Set<String> getInternalFilenames()

public void add(File fileToAdd) throws IOException
{
add(fileToAdd.getName(), Files.map(fileToAdd));
add(fileToAdd.getName(), fileToAdd);
}

public void add(String name, File fileToAdd) throws IOException
{
add(name, Files.map(fileToAdd));
MappedByteBuffer mappedFileBuffer = Files.map(fileToAdd);
try {
add(name, mappedFileBuffer);
} finally {
ByteBufferUtils.unmap(mappedFileBuffer);
Copy link
Copy Markdown
Contributor

@gianm gianm Sep 7, 2016

Choose a reason for hiding this comment

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

try-with-resources preferable here to avoid exception chomping.

Copy link
Copy Markdown
Contributor Author

@leventov leventov Sep 7, 2016

Choose a reason for hiding this comment

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

Yes... also there is #46, and also there is an idea to approach this more fundamentially with ResourceHandlers, discussed in apache/druid#3422. Another option is to move away from ByteBuffers, to e. g. Netty's, Aeron's or Chronicle's alternatives, which are better in many ways. So I close this PR for now anyway.

}
}

public void add(String name, ByteBuffer bufferToAdd) throws IOException
Expand Down