Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AVRO-2511: add case for sync #2392

Merged
merged 3 commits into from
Sep 20, 2023
Merged

AVRO-2511: add case for sync #2392

merged 3 commits into from
Sep 20, 2023

Conversation

clesaec
Copy link
Contributor

@clesaec clesaec commented Jul 25, 2023

What is the purpose of the change

AVRO-2511 :
DataFileWriter flush well data to disk only if using constructor with File, because it use internal class SyncableFileOutputStream, and writer activate "getFD().sync" in order to "Force all system buffers to synchronize with the underlying device".
Here, the idea is to do same when the DataFileWriter is initialized with FileOutputStream instance.
This does still not cover all cases; but at least, this one is ok.

Verifying this change

existing unit tests are enough.

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

@github-actions github-actions bot added the Java Pull Requests for Java binding label Jul 25, 2023
@KalleOlaviNiemitalo
Copy link
Contributor

Should the javadoc mention both Syncable and FileOutputStream?

@opwvhk
Copy link
Contributor

opwvhk commented Sep 14, 2023

Should the javadoc mention both Syncable and FileOutputStream?

Yes, I think it should.

@clesaec
Copy link
Contributor Author

clesaec commented Sep 20, 2023

@KalleOlaviNiemitalo : A Javadoc on private field (line 62 of DataFileWriter) or elsewhere in the code ?

@KalleOlaviNiemitalo
Copy link
Contributor

@clesaec, this one:

avro/lang/java/avro/src/main/java/org/apache/avro/file/DataFileWriter.java

Lines 441 to 448 in 73752a4

/**
* If this writer was instantiated using a File or using an
* {@linkplain Syncable} instance, this method flushes all buffers for this
* writer to disk. In other cases, this method behaves exactly like
* {@linkplain #flush()}.
*
* @throws IOException
*/

The existing text "using a File" seems to refer to public DataFileWriter<D> create(Schema schema, File file) and doesn't quite cover the case where public DataFileWriter<D> create(Schema schema, OutputStream outs) is called with outs being a FileOutputStream.

@clesaec
Copy link
Contributor Author

clesaec commented Sep 20, 2023

the comment already mention Syncable :

  /**
    * If this writer was instantiated using a File or using an 
    * {@linkplain Syncable} instance

Should i write something like

    * If this writer was instantiated using a FileOutputStream or using an 
    * {@linkplain Syncable} instance

?

@KalleOlaviNiemitalo
Copy link
Contributor

"using a File, FileOutputStream, or Syncable instance", I think. With hyperlinks to each type.

@clesaec
Copy link
Contributor Author

clesaec commented Sep 20, 2023

Thanks, done.

@clesaec clesaec merged commit a1e1444 into apache:master Sep 20, 2023
13 checks passed
RanbirK pushed a commit to RanbirK/avro that referenced this pull request May 13, 2024
@clesaec
AVRO-2511: add case for sync (apache#2392)
f84d501 
* AVRO-2511: add case for sync
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@opwvhk opwvhk opwvhk approved these changes

Assignees
No one assigned
Labels
Java Pull Requests for Java binding
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@clesaec @KalleOlaviNiemitalo @opwvhk