You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by John Vines <vi...@apache.org> on 2013/10/28 18:55:49 UTC
Review Request 14995: ACCUMULO-1679 - Purges TFile by rolling the two static
calls into BCFile
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14995/
-----------------------------------------------------------
Review request for accumulo.
Bugs: ACCUMULO-1679
https://issues.apache.org/jira/browse/ACCUMULO-1679
Repository: accumulo
Description
-------
Dirt simple fix which moves the 2 static variables and methods into BCFile. I'm pretty sure we can't go about renaming those constants, but I could be mistaken. General trivialness of this fix is why I'm review boarding it - it seems too easy.
Diffs
-----
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java 2d9f6af
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java e89bb40
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFile.java f2cb326
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFileDumper.java d5b0a1b
Diff: https://reviews.apache.org/r/14995/diff/
Testing
-------
Thanks,
John Vines
Re: Review Request 14995: ACCUMULO-1679 - Purges TFile by rolling the two
static calls into BCFile
Posted by John Vines <vi...@apache.org>.
> On Oct. 28, 2013, 6:46 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java, line 255
> > <https://reviews.apache.org/r/14995/diff/1/?file=372231#file372231line255>
> >
> > How about defining a new attribute "bcfile.fs.output.buffer.size" which can be primarily used, with "tfile" as a fallback for compatibility? (Ditto for the input buffer.) Leaving the tfile ones there will just be confusing after a while.
I wanted to avoid renaming the configuration parameters due to backwards compatibility.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14995/#review27625
-----------------------------------------------------------
On Oct. 28, 2013, 10:09 p.m., John Vines wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14995/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2013, 10:09 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1679
> https://issues.apache.org/jira/browse/ACCUMULO-1679
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Dirt simple fix which moves the 2 static variables and methods into BCFile. I'm pretty sure we can't go about renaming those constants, but I could be mistaken. General trivialness of this fix is why I'm review boarding it - it seems too easy.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java 2d9f6af
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Chunk.java a075d87
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java e89bb40
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFile.java f2cb326
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFileDumper.java d5b0a1b
>
> Diff: https://reviews.apache.org/r/14995/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> John Vines
>
>
Re: Review Request 14995: ACCUMULO-1679 - Purges TFile by rolling the two
static calls into BCFile
Posted by Bill Havanki <bh...@cloudera.com>.
> On Oct. 28, 2013, 2:46 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java, line 255
> > <https://reviews.apache.org/r/14995/diff/1/?file=372231#file372231line255>
> >
> > How about defining a new attribute "bcfile.fs.output.buffer.size" which can be primarily used, with "tfile" as a fallback for compatibility? (Ditto for the input buffer.) Leaving the tfile ones there will just be confusing after a while.
>
> John Vines wrote:
> I wanted to avoid renaming the configuration parameters due to backwards compatibility.
Absolutely, but my thinking was to look for "bcfile" first, and if it isn't there, then "tfile", and if that's not defined either, the default. That would start down the path of eventually deprecating and removing the tfile variations.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14995/#review27625
-----------------------------------------------------------
On Oct. 28, 2013, 6:09 p.m., John Vines wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14995/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2013, 6:09 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1679
> https://issues.apache.org/jira/browse/ACCUMULO-1679
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Dirt simple fix which moves the 2 static variables and methods into BCFile. I'm pretty sure we can't go about renaming those constants, but I could be mistaken. General trivialness of this fix is why I'm review boarding it - it seems too easy.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java 2d9f6af
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Chunk.java a075d87
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java e89bb40
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFile.java f2cb326
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFileDumper.java d5b0a1b
>
> Diff: https://reviews.apache.org/r/14995/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> John Vines
>
>
Re: Review Request 14995: ACCUMULO-1679 - Purges TFile by rolling the two
static calls into BCFile
Posted by John Vines <vi...@apache.org>.
> On Oct. 28, 2013, 6:46 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java, line 255
> > <https://reviews.apache.org/r/14995/diff/1/?file=372231#file372231line255>
> >
> > How about defining a new attribute "bcfile.fs.output.buffer.size" which can be primarily used, with "tfile" as a fallback for compatibility? (Ditto for the input buffer.) Leaving the tfile ones there will just be confusing after a while.
>
> John Vines wrote:
> I wanted to avoid renaming the configuration parameters due to backwards compatibility.
>
> Bill Havanki wrote:
> Absolutely, but my thinking was to look for "bcfile" first, and if it isn't there, then "tfile", and if that's not defined either, the default. That would start down the path of eventually deprecating and removing the tfile variations.
>
> John Vines wrote:
> I'm also not sure if this is a configuration that is supposed to be picked up out of the Accumulo config or the hadoop config
Opened a ticket for this in order to get a bit more discussion happening around it
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14995/#review27625
-----------------------------------------------------------
On Oct. 29, 2013, 9:11 p.m., John Vines wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14995/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 9:11 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1679
> https://issues.apache.org/jira/browse/ACCUMULO-1679
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Dirt simple fix which moves the 2 static variables and methods into BCFile. I'm pretty sure we can't go about renaming those constants, but I could be mistaken. General trivialness of this fix is why I'm review boarding it - it seems too easy.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java 2d9f6af
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Chunk.java a075d87
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java e89bb40
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFile.java f2cb326
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFileDumper.java d5b0a1b
>
> Diff: https://reviews.apache.org/r/14995/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> John Vines
>
>
Re: Review Request 14995: ACCUMULO-1679 - Purges TFile by rolling the two
static calls into BCFile
Posted by John Vines <vi...@apache.org>.
> On Oct. 28, 2013, 6:46 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java, line 255
> > <https://reviews.apache.org/r/14995/diff/1/?file=372231#file372231line255>
> >
> > How about defining a new attribute "bcfile.fs.output.buffer.size" which can be primarily used, with "tfile" as a fallback for compatibility? (Ditto for the input buffer.) Leaving the tfile ones there will just be confusing after a while.
>
> John Vines wrote:
> I wanted to avoid renaming the configuration parameters due to backwards compatibility.
>
> Bill Havanki wrote:
> Absolutely, but my thinking was to look for "bcfile" first, and if it isn't there, then "tfile", and if that's not defined either, the default. That would start down the path of eventually deprecating and removing the tfile variations.
I'm also not sure if this is a configuration that is supposed to be picked up out of the Accumulo config or the hadoop config
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14995/#review27625
-----------------------------------------------------------
On Oct. 28, 2013, 10:09 p.m., John Vines wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14995/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2013, 10:09 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1679
> https://issues.apache.org/jira/browse/ACCUMULO-1679
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Dirt simple fix which moves the 2 static variables and methods into BCFile. I'm pretty sure we can't go about renaming those constants, but I could be mistaken. General trivialness of this fix is why I'm review boarding it - it seems too easy.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java 2d9f6af
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Chunk.java a075d87
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java e89bb40
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFile.java f2cb326
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFileDumper.java d5b0a1b
>
> Diff: https://reviews.apache.org/r/14995/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> John Vines
>
>
Re: Review Request 14995: ACCUMULO-1679 - Purges TFile by rolling the two
static calls into BCFile
Posted by Bill Havanki <bh...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14995/#review27625
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java
<https://reviews.apache.org/r/14995/#comment53667>
How about defining a new attribute "bcfile.fs.output.buffer.size" which can be primarily used, with "tfile" as a fallback for compatibility? (Ditto for the input buffer.) Leaving the tfile ones there will just be confusing after a while.
- Bill Havanki
On Oct. 28, 2013, 1:55 p.m., John Vines wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14995/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2013, 1:55 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1679
> https://issues.apache.org/jira/browse/ACCUMULO-1679
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Dirt simple fix which moves the 2 static variables and methods into BCFile. I'm pretty sure we can't go about renaming those constants, but I could be mistaken. General trivialness of this fix is why I'm review boarding it - it seems too easy.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java 2d9f6af
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java e89bb40
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFile.java f2cb326
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFileDumper.java d5b0a1b
>
> Diff: https://reviews.apache.org/r/14995/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> John Vines
>
>
Re: Review Request 14995: ACCUMULO-1679 - Purges TFile by rolling the two
static calls into BCFile
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14995/#review27910
-----------------------------------------------------------
Ship it!
Seems simple enough. I'd be ok with leaving the tfile parameter in BCFile for backwards compat like you said.
- Josh Elser
On Oct. 29, 2013, 9:11 p.m., John Vines wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14995/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 9:11 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1679
> https://issues.apache.org/jira/browse/ACCUMULO-1679
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Dirt simple fix which moves the 2 static variables and methods into BCFile. I'm pretty sure we can't go about renaming those constants, but I could be mistaken. General trivialness of this fix is why I'm review boarding it - it seems too easy.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java 2d9f6af
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Chunk.java a075d87
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java e89bb40
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFile.java f2cb326
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFileDumper.java d5b0a1b
>
> Diff: https://reviews.apache.org/r/14995/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> John Vines
>
>
Re: Review Request 14995: ACCUMULO-1679 - Purges TFile by rolling the two
static calls into BCFile
Posted by John Vines <vi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14995/
-----------------------------------------------------------
(Updated Oct. 29, 2013, 9:11 p.m.)
Review request for accumulo.
Changes
-------
Fixed random TODO in javadoc.
Shifted former tfile constants and helper methods to be colocated and private
Bugs: ACCUMULO-1679
https://issues.apache.org/jira/browse/ACCUMULO-1679
Repository: accumulo
Description
-------
Dirt simple fix which moves the 2 static variables and methods into BCFile. I'm pretty sure we can't go about renaming those constants, but I could be mistaken. General trivialness of this fix is why I'm review boarding it - it seems too easy.
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java 2d9f6af
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Chunk.java a075d87
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java e89bb40
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFile.java f2cb326
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFileDumper.java d5b0a1b
Diff: https://reviews.apache.org/r/14995/diff/
Testing
-------
Thanks,
John Vines
Re: Review Request 14995: ACCUMULO-1679 - Purges TFile by rolling the two
static calls into BCFile
Posted by Christopher Tubbs <ct...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14995/#review27726
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java
<https://reviews.apache.org/r/14995/#comment53847>
Fix TODO in javadoc
- Christopher Tubbs
On Oct. 28, 2013, 6:09 p.m., John Vines wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14995/
> -----------------------------------------------------------
>
> (Updated Oct. 28, 2013, 6:09 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1679
> https://issues.apache.org/jira/browse/ACCUMULO-1679
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Dirt simple fix which moves the 2 static variables and methods into BCFile. I'm pretty sure we can't go about renaming those constants, but I could be mistaken. General trivialness of this fix is why I'm review boarding it - it seems too easy.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java 2d9f6af
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Chunk.java a075d87
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java e89bb40
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFile.java f2cb326
> core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFileDumper.java d5b0a1b
>
> Diff: https://reviews.apache.org/r/14995/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> John Vines
>
>
Re: Review Request 14995: ACCUMULO-1679 - Purges TFile by rolling the two
static calls into BCFile
Posted by John Vines <vi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14995/
-----------------------------------------------------------
(Updated Oct. 28, 2013, 10:09 p.m.)
Review request for accumulo.
Changes
-------
Updated with patch which removed Chunk as well
Bugs: ACCUMULO-1679
https://issues.apache.org/jira/browse/ACCUMULO-1679
Repository: accumulo
Description
-------
Dirt simple fix which moves the 2 static variables and methods into BCFile. I'm pretty sure we can't go about renaming those constants, but I could be mistaken. General trivialness of this fix is why I'm review boarding it - it seems too easy.
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java 2d9f6af
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Chunk.java a075d87
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/Compression.java e89bb40
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFile.java f2cb326
core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/TFileDumper.java d5b0a1b
Diff: https://reviews.apache.org/r/14995/diff/
Testing
-------
Thanks,
John Vines