[
https://issues.apache.org/jira/browse/AVRO-158?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12768121#action_12768121
]
Philip Zeyliger commented on AVRO-158:
--------------------------------------
Overall, looks good.
Some comments:
bq. "Return the number of records in the file."
You removed "according to its metadata" from this comment. I'm not tied to the
exact wording, but when I ran into this method for the first time, I assumed
that it was going through the entire file and counting records, and thought,
"boy, that's a bad idea". Exposing a bit of information about how it works, or
what it's runtime is would be helpful.
bq. public DataFileWriter(File file, DatumWriter<D> dout)
I don't mind convenience methods for File, but the fact that
DataFileWriter(File, DatumWriter) is so very different from
DataFileWriter(Schema, File, DatumWriter) bugs me a bit. I'd prefer
DataFileWriter.createForAppend(File, DatumWriter) as a factory method. Isn't
it possible to append with a different schema, btw? How does that work?
To answer the question you posed, I think a factory method with an enum, or
just several factory methods.
bq. if (!file.exists())
The Sun style guidelines prefer braces around if statements.
bq. } catch (Exception e) {
If the code happens to throw a RuntimeException (OOM, say), it will get
re-wrapped in another RuntimeException, which seems unnecessary. Perhaps
better to catch NoSuchAlgorithmException explicitly?
bq. testGenericAppend
testGenericAppend looks like it depends on testGenericWrite executing first.
I'm not that familiar with this version of JUnit, but is it a goal that the
individual tests run standalone? In Junit3, setUp() and tearDown() were used
for this.
> Add test to Java bindings for opening a non-empty file object container and
> successfully adding new elements
> ------------------------------------------------------------------------------------------------------------
>
> Key: AVRO-158
> URL: https://issues.apache.org/jira/browse/AVRO-158
> Project: Avro
> Issue Type: Test
> Components: java
> Reporter: Jeff Hammerbacher
> Assignee: Doug Cutting
> Fix For: 1.3.0
>
> Attachments: AVRO-158.patch
>
>
> Avro file object container are mutable and we should test that fact.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
