AVRO-4322: [Java] Use reader/writers constently with model - #3924
Conversation
RyanSkraba
changed the title
af2ee0b to
33b8f8a
Compare
| MyReader() { | ||
| // Use a new data model instead of the singleton in order to avoid modifying the | ||
| // setFastReaderEnabled state for other tests. | ||
| super(null, null, new SpecificData()); |
There was a problem hiding this comment.
OK, it looks like this was a hidden bug -- if we setFastReaderEnabled(true) or setFastReaderEnabled(false) on the singleton instance of SpecificData, it overrides the system property and in other tests we end up not testing what we thought we were testing...
I checked through the code and this looks like the only spot where it gets changed in the singleton.
There was a problem hiding this comment.
Oh wow, that's a bit weird so we were not really testing everything correctly ufff. Oh wait that's Test specific, I see now.
iemejia
left a comment
iemejia
left a comment
There was a problem hiding this comment.
LGTM, thanks for figuring out the reason. It was far from trivial. What about other subclasses, or is this the only one?
| MyReader() { | ||
| // Use a new data model instead of the singleton in order to avoid modifying the | ||
| // setFastReaderEnabled state for other tests. | ||
| super(null, null, new SpecificData()); |
There was a problem hiding this comment.
Oh wow, that's a bit weird so we were not really testing everything correctly ufff. Oh wait that's Test specific, I see now.
|
I checked, and this was the only instance I found, but I raised https://issues.apache.org/jira/browse/AVRO-4326 to find a strategy to make sure we don't introduce new instances. |
2 participants
What is the purpose of the change
We should be using the GenericDatumReader and SpecificDatumReader with the respective models. The mix causes a test failure with fastread.
Verifying this change
This change is already covered by existing tests, such as FastReaderBuilderJavaClassTest.
Documentation