You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@parquet.apache.org by "hellishfire (via GitHub)" <gi...@apache.org> on 2024/09/11 12:43:26 UTC
[I] Should deny further write if writer is aborted [parquet-java]
hellishfire opened a new issue, #3011:
URL: https://github.com/apache/parquet-java/issues/3011
### Describe the bug, including details regarding any error messages, version, and platform.
After [PARQUET-2437](https://github.com/apache/parquet-java/pull/1285), InternalParquetRecordWriter will switch to aborted state if write throws, but still accept further writes even though these writes will not be flushed during close, leaving the file incomplete. Users will not notice the corrupted file until they actually read the file.
Should we check for aborted state and reject further writes to avoid such surprises?
@gszadovszky
### Component(s)
_No response_
--
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@parquet.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [I] Should deny further write if ParquetWriter is aborted [parquet-java]
Posted by "gszadovszky (via GitHub)" <gi...@apache.org>.
gszadovszky commented on issue #3011:
URL: https://github.com/apache/parquet-java/issues/3011#issuecomment-2367540348
@hellishfire, my bad, should have explained in more details. In many cases, Parquet writers are used in a try-with-resources construct. It invokes `close` in any case including exceptions thrown during the writes. `close` invokes flush by default which actually does some writing. It may cause issues like throwing exceptions while closing which is not always a problem. But, if you would use e.g. direct memory via the `ByteBufferAllocator` interface, it needs to be released and if fails, it may lead to memory leaks.
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [I] Should deny further write if ParquetWriter is aborted [parquet-java]
Posted by "hellishfire (via GitHub)" <gi...@apache.org>.
hellishfire commented on issue #3011:
URL: https://github.com/apache/parquet-java/issues/3011#issuecomment-2363825953
> > We found that some users still try to write after exception, then successfully close the file, only to find out later that the file can not be opened for read because of missing file footer.
> > Perhaps it's better to deny any further write to make it fool-proof?
>
> I don't think parquet-java is fool-proof in any ways. And how do you think we should handle the case of continue writing with a writer that is already "aborted"? Throw yet another exception? I am not against it but cannot see the point. Also, it shall not significantly impact write performance.
>
> > Should we narrow the try-catch scope to exclude exceptions thrown by writeSupport (i.e. illegal input values denied by writeSupport), which is irrelevant to actual file io operation?
>
> It would make sense but I am not sure if we can 100% differentiate the potential exceptions and whether we already had impact on the file writing at the throwing time or not.
>
> Feel free to put up a PR if you would like to work on this. I'm happy to review.
OK. Would it be possible for you to elaborate a bit on why aborted state was introduced in the first place? Like, which user case triggered a bug that led you to fix it? It was not explained clearly in your commit message, so I somewhat lack the context of that 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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [I] Should deny further write if ParquetWriter is aborted [parquet-java]
Posted by "hellishfire (via GitHub)" <gi...@apache.org>.
hellishfire commented on issue #3011:
URL: https://github.com/apache/parquet-java/issues/3011#issuecomment-2362692613
On another note, in InternalParquetRecordWriter.java
```
public void write(T value) throws IOException, InterruptedException {
try {
writeSupport.write(value);
++recordCount;
checkBlockSizeReached();
} catch (Throwable t) {
aborted = true;
throw t;
}
}
```
Should we narrow the try-catch scope to exclude exceptions thrown by writeSupport (i.e. illegal input values denied by writeSupport), which is irrelevant to actual file io operation?
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [I] Should deny further write if ParquetWriter is aborted [parquet-java]
Posted by "gszadovszky (via GitHub)" <gi...@apache.org>.
gszadovszky commented on issue #3011:
URL: https://github.com/apache/parquet-java/issues/3011#issuecomment-2358317475
@hellishfire, How users would not notice the corruption when the related exception is thrown? My change is related to auto-closing to not to write anything automatically (at closing) after an exception happened. The users of the API shall not continue writing data after an exception.
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [I] Should deny further write if ParquetWriter is aborted [parquet-java]
Posted by "hellishfire (via GitHub)" <gi...@apache.org>.
hellishfire commented on issue #3011:
URL: https://github.com/apache/parquet-java/issues/3011#issuecomment-2362688302
> @hellishfire, How users would not notice the corruption when the related exception is thrown? My change is related to auto-closing to not to write anything automatically (at closing) after an exception happened. The users of the API shall not continue writing data after an exception.
We found that some users still try to write after exception, then successfully close the file, only to find out later that the file can not be opened for read because of missing file footer.
Perhaps it's better to deny any write to make it fool-proof?
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [I] Should deny further write if ParquetWriter is aborted [parquet-java]
Posted by "gszadovszky (via GitHub)" <gi...@apache.org>.
gszadovszky commented on issue #3011:
URL: https://github.com/apache/parquet-java/issues/3011#issuecomment-2363199403
> We found that some users still try to write after exception, then successfully close the file, only to find out later that the file can not be opened for read because of missing file footer.
Perhaps it's better to deny any further write to make it fool-proof?
I don't think parquet-java is fool-proof in any ways. And how do you think we should handle the case of continue writing with a writer that is already "aborted"? Throw yet another exception? I am not against it but cannot see the point. Also, it shall not significantly impact write performance.
> Should we narrow the try-catch scope to exclude exceptions thrown by writeSupport (i.e. illegal input values denied by writeSupport), which is irrelevant to actual file io operation?
It would make sense but I am not sure if we can 100% differentiate the potential exceptions and whether we already had impact on the file writing at the throwing time or not.
Feel free to put up a PR if you would like to work on this. I'm happy to review.
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [I] Should deny further write if ParquetWriter is aborted [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac closed issue #3011: Should deny further write if ParquetWriter is aborted
URL: https://github.com/apache/parquet-java/issues/3011
--
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@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org