[GitHub] [nifi] ijokarumawak commented on a change in pull request #3573: NIFI-6419: Fixed AvroWriter single record with external schema result…

[GitBox <gi...@apache.org>]

ijokarumawak commented on a change in pull request #3573: NIFI-6419: Fixed AvroWriter single record with external schema result…
URL: https://github.com/apache/nifi/pull/3573#discussion_r301860522
 
 

 ##########
 File path: nifi-nar-bundles/nifi-standard-services/nifi-record-serialization-services-bundle/nifi-record-serialization-services/src/main/java/org/apache/nifi/avro/WriteAvroResultWithExternalSchema.java
 ##########
 @@ -78,14 +78,19 @@ protected void onBeginRecordSet() throws IOException {
     @Override
     public Map<String, String> writeRecord(final Record record) throws IOException {
         // If we are not writing an active record set, then we need to ensure that we write the
-        // schema information.
+        // schema information at the beginning and call flush at the end.
         if (!isActiveRecordSet()) {
             flush();
             schemaAccessWriter.writeHeader(recordSchema, getOutputStream());
         }
 
         final GenericRecord rec = AvroTypeUtil.createAvroRecord(record, avroSchema);
         datumWriter.write(rec, encoder);
+
+        if (!isActiveRecordSet()) {
+            flush();
 
 Review comment:
   Hi, thank you both for providing the fix! Please let me join the discussion :)
   
   > I'd prefer flush() here because write(RecordSet recordSet) in AbstractRecordSetWriter handles its own beginRecordSet() / finishRecordSet() calls (and flush() is being called there). So I think write(Record record) should do the same.
   
   @turcsanyip, probably I'm missing something, but I don't find any `flush()` call from AbstractRecordSetWriter `write(RecordSet recordSet)`, `beginRecordSet()` or `finishRecordSet()`. If no flush is called from there, the redundancy you worried about doesn't happen for the `write(RecordSet)` path even if we add `flush()` to `close()` at WriteAvroResultWithExternalSchema. Please correct me if I'm wrong on this.
   
   Then, I'd prefer just adding `flush()` to `close()` as @pcgrenier proposed at #3568 
   
   If both of you agree, I can merge #3573 first to have the unit tests, then #3568 to overwrite how to call flush(). Then commit history will look like this. How do you guys think?
   https://github.com/ijokarumawak/nifi/commits/nifi-6419

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services