You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2022/04/12 05:04:29 UTC
[GitHub] [avro] rbalamohan opened a new pull request, #1639: AVRO-3482: Reuse MAGIC in DataFileReader
rbalamohan opened a new pull request, #1639:
URL: https://github.com/apache/avro/pull/1639
DataFileReader reads magic information twice. seek(0) is invoked
twice due to this. In cloud object stores, seeking back to 0 will
cause it to fall back to "random IO policy". Example of this is
S3A connector for s3. This causes suboptimal reads in object stores.
Refactoring in the patch addresses this case by reusing MAGIC.
### Jira
https://issues.apache.org/jira/browse/AVRO-3482
### Tests
- Existing test cases cover this refactoring.
### Commits
### Documentation
N/A
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] steveloughran commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
steveloughran commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r852210974
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileReader12.java:
##########
@@ -61,6 +61,7 @@ public DataFileReader12(SeekableInput sin, DatumReader<D> reader) throws IOExcep
this.in = new DataFileReader.SeekableInputStream(sin);
byte[] magic = new byte[4];
+ in.seek(0); // seek to 0 to read magic header
in.read(magic);
Review Comment:
what if fewer bytes come back? why not a readFully()?
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileStream.java:
##########
@@ -97,18 +97,30 @@ protected DataFileStream(DatumReader<D> reader) throws IOException {
this.reader = reader;
}
- /** Initialize the stream by reading from its head. */
- void initialize(InputStream in) throws IOException {
- this.header = new Header();
- this.vin = DecoderFactory.get().binaryDecoder(in, vin);
+ byte[] readMagic() throws IOException {
+ if (this.vin == null) {
+ throw new IOException("InputStream is not initialized");
+ }
byte[] magic = new byte[DataFileConstants.MAGIC.length];
try {
vin.readFixed(magic); // read magic
} catch (IOException e) {
throw new IOException("Not an Avro data file.", e);
Review Comment:
always nice to include the error string of the nested exception, as there may be many other causes of problems than just file type
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] rbalamohan commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
rbalamohan commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r859201620
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileStream.java:
##########
@@ -97,18 +97,30 @@ protected DataFileStream(DatumReader<D> reader) throws IOException {
this.reader = reader;
}
- /** Initialize the stream by reading from its head. */
- void initialize(InputStream in) throws IOException {
- this.header = new Header();
- this.vin = DecoderFactory.get().binaryDecoder(in, vin);
+ byte[] readMagic() throws IOException {
+ if (this.vin == null) {
+ throw new IOException("InputStream is not initialized");
+ }
byte[] magic = new byte[DataFileConstants.MAGIC.length];
try {
vin.readFixed(magic); // read magic
} catch (IOException e) {
throw new IOException("Not an Avro data file.", e);
}
+ return magic;
+ }
+
+ void validateMagic(byte[] magic) throws InvalidAvroMagicException {
if (!Arrays.equals(DataFileConstants.MAGIC, magic))
throw new InvalidAvroMagicException("Not an Avro data file.");
+ }
+
+ /** Initialize the stream by reading from its head. */
+ void initialize(InputStream in, byte[] magic) throws IOException {
Review Comment:
DataFileReader extends this class and making it "private" would cause issue in accessing this method from DataFileReader.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] steveloughran commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
steveloughran commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r854529247
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileStream.java:
##########
@@ -97,18 +97,30 @@ protected DataFileStream(DatumReader<D> reader) throws IOException {
this.reader = reader;
}
- /** Initialize the stream by reading from its head. */
- void initialize(InputStream in) throws IOException {
- this.header = new Header();
- this.vin = DecoderFactory.get().binaryDecoder(in, vin);
+ byte[] readMagic() throws IOException {
+ if (this.vin == null) {
+ throw new IOException("InputStream is not initialized");
+ }
byte[] magic = new byte[DataFileConstants.MAGIC.length];
try {
vin.readFixed(magic); // read magic
} catch (IOException e) {
throw new IOException("Not an Avro data file.", e);
Review Comment:
> Adding the exception cause adds that message. Let's do this the same as in line 108.
yes, if the whole exception history is printed. no if only the first error string gets logged, or the chain of causes gets stripped after a couple of wrappings (hello hive!). if the filename is known, that's handy to include too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] opwvhk commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
opwvhk commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r852594859
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileStream.java:
##########
@@ -97,18 +97,30 @@ protected DataFileStream(DatumReader<D> reader) throws IOException {
this.reader = reader;
}
- /** Initialize the stream by reading from its head. */
- void initialize(InputStream in) throws IOException {
- this.header = new Header();
- this.vin = DecoderFactory.get().binaryDecoder(in, vin);
+ byte[] readMagic() throws IOException {
+ if (this.vin == null) {
+ throw new IOException("InputStream is not initialized");
+ }
byte[] magic = new byte[DataFileConstants.MAGIC.length];
try {
vin.readFixed(magic); // read magic
} catch (IOException e) {
throw new IOException("Not an Avro data file.", e);
Review Comment:
Adding the exception cause already adds that message. We'll done.
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileStream.java:
##########
@@ -97,18 +97,30 @@ protected DataFileStream(DatumReader<D> reader) throws IOException {
this.reader = reader;
}
- /** Initialize the stream by reading from its head. */
- void initialize(InputStream in) throws IOException {
- this.header = new Header();
- this.vin = DecoderFactory.get().binaryDecoder(in, vin);
+ byte[] readMagic() throws IOException {
+ if (this.vin == null) {
+ throw new IOException("InputStream is not initialized");
+ }
byte[] magic = new byte[DataFileConstants.MAGIC.length];
try {
vin.readFixed(magic); // read magic
} catch (IOException e) {
throw new IOException("Not an Avro data file.", e);
Review Comment:
Adding the exception cause already adds that message. Well done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] rbalamohan commented on pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
rbalamohan commented on PR #1639:
URL: https://github.com/apache/avro/pull/1639#issuecomment-1097285246
@opwvhk : Thank you for the review and approval. Can you please let me know if there is any additional process involved for merging this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] rbalamohan commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
rbalamohan commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r852468031
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileStream.java:
##########
@@ -97,18 +97,30 @@ protected DataFileStream(DatumReader<D> reader) throws IOException {
this.reader = reader;
}
- /** Initialize the stream by reading from its head. */
- void initialize(InputStream in) throws IOException {
- this.header = new Header();
- this.vin = DecoderFactory.get().binaryDecoder(in, vin);
+ byte[] readMagic() throws IOException {
+ if (this.vin == null) {
+ throw new IOException("InputStream is not initialized");
+ }
byte[] magic = new byte[DataFileConstants.MAGIC.length];
try {
vin.readFixed(magic); // read magic
} catch (IOException e) {
throw new IOException("Not an Avro data file.", e);
Review Comment:
+1. Didn't want to make additional changes to existing code to keep the changes minimal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] opwvhk commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
opwvhk commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r852594859
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileStream.java:
##########
@@ -97,18 +97,30 @@ protected DataFileStream(DatumReader<D> reader) throws IOException {
this.reader = reader;
}
- /** Initialize the stream by reading from its head. */
- void initialize(InputStream in) throws IOException {
- this.header = new Header();
- this.vin = DecoderFactory.get().binaryDecoder(in, vin);
+ byte[] readMagic() throws IOException {
+ if (this.vin == null) {
+ throw new IOException("InputStream is not initialized");
+ }
byte[] magic = new byte[DataFileConstants.MAGIC.length];
try {
vin.readFixed(magic); // read magic
} catch (IOException e) {
throw new IOException("Not an Avro data file.", e);
Review Comment:
Adding the exception cause adds that message. Let's do this the same as in line 108.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] rbalamohan commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
rbalamohan commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r858536753
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileReader12.java:
##########
@@ -61,6 +61,7 @@ public DataFileReader12(SeekableInput sin, DatumReader<D> reader) throws IOExcep
this.in = new DataFileReader.SeekableInputStream(sin);
byte[] magic = new byte[4];
+ in.seek(0); // seek to 0 to read magic header
in.read(magic);
Review Comment:
since magic has been already initialized to 4 bytes, partial reads will anyways not match the magic. This wouldn't be an issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg merged pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
thiru-mg merged PR #1639:
URL: https://github.com/apache/avro/pull/1639
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] opwvhk commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
opwvhk commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r848022929
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileReader.java:
##########
@@ -69,10 +69,9 @@ public static <D> FileReader<D> openReader(SeekableInput in, DatumReader<D> read
length -= bytesRead;
offset += bytesRead;
}
- in.seek(0);
if (Arrays.equals(MAGIC, magic)) // current format
- return new DataFileReader<>(in, reader);
+ return newreturn new DataFileReader<>(in, reader, magic);
Review Comment:
What is the purpose of `newreturn`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] rbalamohan commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
rbalamohan commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r848034895
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileReader.java:
##########
@@ -69,10 +69,9 @@ public static <D> FileReader<D> openReader(SeekableInput in, DatumReader<D> read
length -= bytesRead;
offset += bytesRead;
}
- in.seek(0);
if (Arrays.equals(MAGIC, magic)) // current format
- return new DataFileReader<>(in, reader);
+ return newreturn new DataFileReader<>(in, reader, magic);
Review Comment:
Something went wrong in applying the patch in the branch being uploaded for PR. Let me fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] rbalamohan commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
rbalamohan commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r858537526
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileStream.java:
##########
@@ -97,18 +97,30 @@ protected DataFileStream(DatumReader<D> reader) throws IOException {
this.reader = reader;
}
- /** Initialize the stream by reading from its head. */
- void initialize(InputStream in) throws IOException {
- this.header = new Header();
- this.vin = DecoderFactory.get().binaryDecoder(in, vin);
+ byte[] readMagic() throws IOException {
+ if (this.vin == null) {
+ throw new IOException("InputStream is not initialized");
+ }
byte[] magic = new byte[DataFileConstants.MAGIC.length];
try {
vin.readFixed(magic); // read magic
} catch (IOException e) {
throw new IOException("Not an Avro data file.", e);
Review Comment:
Filename isn't available. Null check has been added as a safeguard only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r858812769
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileStream.java:
##########
@@ -97,18 +97,30 @@ protected DataFileStream(DatumReader<D> reader) throws IOException {
this.reader = reader;
}
- /** Initialize the stream by reading from its head. */
- void initialize(InputStream in) throws IOException {
- this.header = new Header();
- this.vin = DecoderFactory.get().binaryDecoder(in, vin);
+ byte[] readMagic() throws IOException {
+ if (this.vin == null) {
+ throw new IOException("InputStream is not initialized");
+ }
byte[] magic = new byte[DataFileConstants.MAGIC.length];
try {
vin.readFixed(magic); // read magic
} catch (IOException e) {
throw new IOException("Not an Avro data file.", e);
}
+ return magic;
+ }
+
+ void validateMagic(byte[] magic) throws InvalidAvroMagicException {
if (!Arrays.equals(DataFileConstants.MAGIC, magic))
throw new InvalidAvroMagicException("Not an Avro data file.");
+ }
+
+ /** Initialize the stream by reading from its head. */
+ void initialize(InputStream in, byte[] magic) throws IOException {
Review Comment:
I'm sorry I should have been more complete last time. Can we make these two `initialize` functions be `private`, too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] rbalamohan commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
rbalamohan commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r852467246
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileReader12.java:
##########
@@ -61,6 +61,7 @@ public DataFileReader12(SeekableInput sin, DatumReader<D> reader) throws IOExcep
this.in = new DataFileReader.SeekableInputStream(sin);
byte[] magic = new byte[4];
+ in.seek(0); // seek to 0 to read magic header
in.read(magic);
Review Comment:
SeekableInputStream extends InputStream and doesn't have readFully.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] opwvhk commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
opwvhk commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r852595847
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileReader12.java:
##########
@@ -61,6 +61,7 @@ public DataFileReader12(SeekableInput sin, DatumReader<D> reader) throws IOExcep
this.in = new DataFileReader.SeekableInputStream(sin);
byte[] magic = new byte[4];
+ in.seek(0); // seek to 0 to read magic header
in.read(magic);
Review Comment:
If fewer bytes come back, `read()` returns another number than 4 (this is discarded), and the leftover part of the array contains zeroes.
This fails the magic check later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] rbalamohan commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
rbalamohan commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r858538519
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileStream.java:
##########
@@ -97,18 +97,30 @@ protected DataFileStream(DatumReader<D> reader) throws IOException {
this.reader = reader;
}
- /** Initialize the stream by reading from its head. */
- void initialize(InputStream in) throws IOException {
- this.header = new Header();
- this.vin = DecoderFactory.get().binaryDecoder(in, vin);
+ byte[] readMagic() throws IOException {
+ if (this.vin == null) {
+ throw new IOException("InputStream is not initialized");
+ }
byte[] magic = new byte[DataFileConstants.MAGIC.length];
try {
vin.readFixed(magic); // read magic
} catch (IOException e) {
throw new IOException("Not an Avro data file.", e);
}
+ return magic;
+ }
+
+ void validateMagic(byte[] magic) throws InvalidAvroMagicException {
if (!Arrays.equals(DataFileConstants.MAGIC, magic))
throw new InvalidAvroMagicException("Not an Avro data file.");
+ }
+
+ /** Initialize the stream by reading from its head. */
+ void initializeWithMagic(InputStream in, byte[] magic) throws IOException {
Review Comment:
Thanks @thiru-mg . I have fixed this in recent commit.
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileReader.java:
##########
@@ -128,15 +127,20 @@ public DataFileReader(File file, DatumReader<D> reader) throws IOException {
* <pre/>
*/
public DataFileReader(SeekableInput sin, DatumReader<D> reader) throws IOException {
- this(sin, reader, false);
+ this(sin, reader, false, null);
+ }
+
+ public DataFileReader(SeekableInput sin, DatumReader<D> reader, byte[] magic) throws IOException {
Review Comment:
Thanks @thiru-mg . I have fixed this in recent commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] opwvhk commented on pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
opwvhk commented on PR #1639:
URL: https://github.com/apache/avro/pull/1639#issuecomment-1099961489
Hi @rbalamohan,
The "additional process" is approval & merge by a committer (I'm a contributor). I'm hoping my opinion carries some weight, but I really don't know.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] rbalamohan commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
rbalamohan commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r852467246
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileReader12.java:
##########
@@ -61,6 +61,7 @@ public DataFileReader12(SeekableInput sin, DatumReader<D> reader) throws IOExcep
this.in = new DataFileReader.SeekableInputStream(sin);
byte[] magic = new byte[4];
+ in.seek(0); // seek to 0 to read magic header
in.read(magic);
Review Comment:
SeekableInputStream extends InputStream and doesn't have readFully. Also, there is any mismatch, it anyways throws the exception in the next line.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] opwvhk commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
opwvhk commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r852595847
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileReader12.java:
##########
@@ -61,6 +61,7 @@ public DataFileReader12(SeekableInput sin, DatumReader<D> reader) throws IOExcep
this.in = new DataFileReader.SeekableInputStream(sin);
byte[] magic = new byte[4];
+ in.seek(0); // seek to 0 to read magic header
in.read(magic);
Review Comment:
~If fewer bytes come back, `read()` returns another number than 4 (this is discarded), and the leftover part of the array contains zeroes.~
~This fails the magic check later.~
The mismatch @rbalamohan mentioned only occurs with network traffic, and then only if the first part of the network packet contains an application specific header of up to 1459 bytes, fragmenting the first 4 bytes of the avro file.
Though unlikely, it's probably best to implement `readFully` here as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg commented on a diff in pull request #1639: AVRO-3482: Reuse MAGIC in DataFileReader
Posted by GitBox <gi...@apache.org>.
thiru-mg commented on code in PR #1639:
URL: https://github.com/apache/avro/pull/1639#discussion_r858275569
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileStream.java:
##########
@@ -97,18 +97,30 @@ protected DataFileStream(DatumReader<D> reader) throws IOException {
this.reader = reader;
}
- /** Initialize the stream by reading from its head. */
- void initialize(InputStream in) throws IOException {
- this.header = new Header();
- this.vin = DecoderFactory.get().binaryDecoder(in, vin);
+ byte[] readMagic() throws IOException {
+ if (this.vin == null) {
+ throw new IOException("InputStream is not initialized");
+ }
byte[] magic = new byte[DataFileConstants.MAGIC.length];
try {
vin.readFixed(magic); // read magic
} catch (IOException e) {
throw new IOException("Not an Avro data file.", e);
}
+ return magic;
+ }
+
+ void validateMagic(byte[] magic) throws InvalidAvroMagicException {
if (!Arrays.equals(DataFileConstants.MAGIC, magic))
throw new InvalidAvroMagicException("Not an Avro data file.");
+ }
+
+ /** Initialize the stream by reading from its head. */
+ void initializeWithMagic(InputStream in, byte[] magic) throws IOException {
Review Comment:
To my taste, I would have left this function named `initialize`. The reason is there is a pattern followed throughout to call the non-constructor initializers `initialize()`. But I wouldn't insist on it.
##########
lang/java/avro/src/main/java/org/apache/avro/file/DataFileReader.java:
##########
@@ -128,15 +127,20 @@ public DataFileReader(File file, DatumReader<D> reader) throws IOException {
* <pre/>
*/
public DataFileReader(SeekableInput sin, DatumReader<D> reader) throws IOException {
- this(sin, reader, false);
+ this(sin, reader, false, null);
+ }
+
+ public DataFileReader(SeekableInput sin, DatumReader<D> reader, byte[] magic) throws IOException {
Review Comment:
Since the magic is embedded in the file and nobody outside the class (let alone the package) is likely to have the magic, it is better to make this constructor private. If we come across a valid use case in the future, we can then make it public.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org