Skip to content

Set the default buffer size (if not specified) to 128k #66

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

childsb
Copy link
Contributor

@childsb childsb commented Nov 18, 2013

If no configuration value is specified for io.file.buffer.size, set it to 128k.

@jayunit100
Copy link
Contributor

preapproved +1 , pending addition of a Log.info("Setting buffer size to ...........") statement. otherwise this is really hard to debug at scale in case any issues.

@wattsteve
Copy link

I'd like to see unit tests attached to each pull request. Perhaps, take Jay's pull 55 and merge it with this?

@jayunit100
Copy link
Contributor

good point steve :) Retracting my +1 until you merge my unit test :) :) #65 (or just copy the code - i dont care about the pull request, just copy the code into this commit )

@childsb
Copy link
Contributor Author

childsb commented Nov 18, 2013

@jayunit100 @wattsteve updated this pull with your requests.

@wattsteve
Copy link

+1

@jayunit100
Copy link
Contributor

nice one brad +1. real quick before you merge: can you confirm that the unit test passes now ? I've confirmed that it fails on the old shim which uses a 4 or 8K default.

@childsb
Copy link
Contributor Author

childsb commented Nov 18, 2013

@jayunit100 : that unit test isn't passing. can you fix + submit a change?

@jayunit100
Copy link
Contributor

hmmmmmmm okay will patch it up . you want me to commit to your childsb/branch ?

@jayunit100
Copy link
Contributor

hola. The test is failing for all the right reasons: The default buffering is still 4096 bytes. :)... You need to update your "if ..." statement to override the default of 4096, not just to override in case of null/empty. Hadoop will provide a very low 4096 byte default, and that will (Rightly) cause this test to fail.

@jayunit100
Copy link
Contributor

Well, nevermind. I guess i will patch the test so that it confirms that the io.file.buffer.size parameter is honored and let you folks work out the details of wether or not we should even ALLOW the 4K byte write buffer, or just override it entirely.

@jayunit100
Copy link
Contributor

//this test will pass, edit (bufferSize) to be larger, once we decide what gluster should allow as a minium.
@Test
public void testBufferSpill() throws Exception {
    Path out = new Path("a");
    final Integer buffersize = fs.getConf().getInt("io.file.buffer.size", -1);
    FSDataOutputStream os = fs.create(out);

    int written=0;
    /**
     * Assert that writes smaller than 10KB are NOT spilled to disk
     */
    while(written<buffersize){
        os.write("ASDF".getBytes());
        written+="ASDF".getBytes().length;
        //now, we expect
        Assert.assertTrue("asserting that file not written yet...",fs.getLength(out)==0);
    }
    os.flush();
    Assert.assertTrue("asserting that is now written... ",fs.getLength(out)>=buffersize);

    os.close();
    fs.delete(out);
}

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.

3 participants