[jira] [Commented] (CASSANDRA-15225) FileUtils.close() does not handle non-IOException
[ https://issues.apache.org/jira/browse/CASSANDRA-15225?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16901983#comment-16901983 ] Liudmila Kornilova commented on CASSANDRA-15225: [~benedict], done! > FileUtils.close() does not handle non-IOException > - > > Key: CASSANDRA-15225 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15225 > Project: Cassandra > Issue Type: Bug > Components: Local/SSTable >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 3.0.x, 3.11.x, 4.0.x > > Time Spent: 10m > Remaining Estimate: 0h > > This can lead to {{close}} not being invoked on remaining items -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15225) FileUtils.close() does not handle non-IOException
[ https://issues.apache.org/jira/browse/CASSANDRA-15225?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16901969#comment-16901969 ] Benedict commented on CASSANDRA-15225: -- Thanks [~Override]. One tiny nit: {{maybeFail}} doesn't require the null check, you can simply invoke it, and if the parameter is {{null}} nothing will happen. If you want to prepare the final patch for commit, by squashing, adding a CHANGES.txt entry, and adding a commit message of the form described on CASSANDRA-15246, I'll commit it to trunk. Welcome to the contributor community! > FileUtils.close() does not handle non-IOException > - > > Key: CASSANDRA-15225 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15225 > Project: Cassandra > Issue Type: Bug > Components: Local/SSTable >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 3.0.x, 3.11.x, 4.0.x > > Time Spent: 10m > Remaining Estimate: 0h > > This can lead to {{close}} not being invoked on remaining items -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15225) FileUtils.close() does not handle non-IOException
[ https://issues.apache.org/jira/browse/CASSANDRA-15225?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16901962#comment-16901962 ] Liudmila Kornilova commented on CASSANDRA-15225: Thank you for reviewing the code, [~benedict] It sounds like a better approach because some {{Throwables}} should not be re-thrown as checked (e.g. {{Error}} is a "serious problems that a reasonable application should not try to catch"). Also no additional wrappers I pushed third commit > FileUtils.close() does not handle non-IOException > - > > Key: CASSANDRA-15225 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15225 > Project: Cassandra > Issue Type: Bug > Components: Local/SSTable >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 3.0.x, 3.11.x, 4.0.x > > Time Spent: 10m > Remaining Estimate: 0h > > This can lead to {{close}} not being invoked on remaining items -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15225) FileUtils.close() does not handle non-IOException
[ https://issues.apache.org/jira/browse/CASSANDRA-15225?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16901269#comment-16901269 ] Benedict commented on CASSANDRA-15225: -- Thanks for the patch [~Override]. There's a third option, namely accumulating the throwable in a {{Throwable}} variable, and maintaining the single catch clause. We have a utility method, {{Throwables.maybeFail}} that takes a checked exception class, and rethrows the exception as its checked-type if possible, or as an unchecked type if possible, and otherwise wraps it in a {{RuntimeException}}. Does that sound reasonable to you? > FileUtils.close() does not handle non-IOException > - > > Key: CASSANDRA-15225 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15225 > Project: Cassandra > Issue Type: Bug > Components: Local/SSTable >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 3.0.x, 3.11.x, 4.0.x > > Time Spent: 10m > Remaining Estimate: 0h > > This can lead to {{close}} not being invoked on remaining items -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15225) FileUtils.close() does not handle non-IOException
[ https://issues.apache.org/jira/browse/CASSANDRA-15225?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16899886#comment-16899886 ] Liudmila Kornilova commented on CASSANDRA-15225: {quote}2) Log statement has only one '{}'. I don't think it will print exception. I prefer caller to handle the logging part. {quote} I checked this. An exception is extracted here ch.qos.logback.classic.spi.LoggingEvent:49 and it is printed {quote}unless you want to make any changes in light of [~n.v.harikrishna]'s feedback? {quote} I decided to change the code such that new exception is created. I see 2 advantages of this version: 1. {{Throwable}} will not be lost if it appears before {{IOException}} or {{IOException}} does not appear at all. {{Throwable}} will be rethrown as IOException with cause. 2. Catch clauses become identical and they can be collapsed to one {{Throwable}} clause I pushed a separate commit so you can see the difference. I can squash commits if you wish > FileUtils.close() does not handle non-IOException > - > > Key: CASSANDRA-15225 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15225 > Project: Cassandra > Issue Type: Bug > Components: Local/SSTable >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 3.0.x, 3.11.x, 4.0.x > > Time Spent: 10m > Remaining Estimate: 0h > > This can lead to {{close}} not being invoked on remaining items -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15225) FileUtils.close() does not handle non-IOException
[ https://issues.apache.org/jira/browse/CASSANDRA-15225?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16897920#comment-16897920 ] Benedict commented on CASSANDRA-15225: -- {quote}2) Log statement has only one '{}'. I don't think it will print exception. I prefer caller to handle the logging part. {quote} This (the way [~Override] has it) is the correct way to print exceptions; otherwise only their {{toString()}} method is invoked. {quote}1) Insteasd of modifying already thrown exception, better to create new exception and call _newException_.addSuppressed. When the caller calls _newException_.getSuppressed, accurate list exceptions are returned. {quote} I don't think it is correct to have add exceptions with {{addSuppressed}}. It _is_ the case that often a new exception is created in any place exceptions are caught, so that the re-throwing method is captured in the trace, however you would always want a {{cause}} in preference to adding a member of {{getSuppressed}}. However, I don't think this anyway always adds value. If I were personally writing it, I would likely have done it the way [~Override] has done. There's an alternative, which is to create a new exception whose {{cause}} is the first caught exception, and to {{addSuppressed}} any remaining exceptions to this new exception. But I think it's preferable to rethrow the original exception here, to avoid polluting the printed stack trace, and minimise the risk that "helpful" tools truncate some of this trace [~Override]: LGTM, I will merge it alongside your other patch and some others I will be reviewing hopefully today or tomorrow. > FileUtils.close() does not handle non-IOException > - > > Key: CASSANDRA-15225 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15225 > Project: Cassandra > Issue Type: Bug > Components: Local/SSTable >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 3.0.x, 3.11.x, 4.0.x > > Time Spent: 10m > Remaining Estimate: 0h > > This can lead to {{close}} not being invoked on remaining items -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15225) FileUtils.close() does not handle non-IOException
[ https://issues.apache.org/jira/browse/CASSANDRA-15225?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16897432#comment-16897432 ] Venkata Harikrishna Nukala commented on CASSANDRA-15225: 1) Insteasd of modifying already thrown exception, better to create new exception and call _newException_.addSuppressed. When the caller calls _newException_.getSuppressed, accurate list exceptions are returned. 2) Log statement has only one '{}'. I don't think it will print exception. I prefer caller to handle the logging part. > FileUtils.close() does not handle non-IOException > - > > Key: CASSANDRA-15225 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15225 > Project: Cassandra > Issue Type: Bug > Components: Local/SSTable >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 3.0.x, 3.11.x, 4.0.x > > Time Spent: 10m > Remaining Estimate: 0h > > This can lead to {{close}} not being invoked on remaining items -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15225) FileUtils.close() does not handle non-IOException
[ https://issues.apache.org/jira/browse/CASSANDRA-15225?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16888607#comment-16888607 ] Liudmila Kornilova commented on CASSANDRA-15225: Hi [~n.v.harikrishna], Done All IOExceptions except first one are also added to suppressed (see [updated commit|https://github.com/apache/cassandra/pull/332/files]) > FileUtils.close() does not handle non-IOException > - > > Key: CASSANDRA-15225 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15225 > Project: Cassandra > Issue Type: Bug > Components: Local/SSTable >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 3.0.x, 3.11.x, 4.0.x > > Time Spent: 10m > Remaining Estimate: 0h > > This can lead to {{close}} not being invoked on remaining items -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
[jira] [Commented] (CASSANDRA-15225) FileUtils.close() does not handle non-IOException
[ https://issues.apache.org/jira/browse/CASSANDRA-15225?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16888298#comment-16888298 ] Venkata Harikrishna Nukala commented on CASSANDRA-15225: [~Override] Instead of delivering last exception as IOException, can we use {{Throwable.addSuppressed()}} ? cc: [~benedict] > FileUtils.close() does not handle non-IOException > - > > Key: CASSANDRA-15225 > URL: https://issues.apache.org/jira/browse/CASSANDRA-15225 > Project: Cassandra > Issue Type: Bug > Components: Local/SSTable >Reporter: Benedict >Assignee: Liudmila Kornilova >Priority: Normal > Labels: pull-request-available > Fix For: 3.0.x, 3.11.x, 4.0.x > > Time Spent: 10m > Remaining Estimate: 0h > > This can lead to {{close}} not being invoked on remaining items -- This message was sent by Atlassian JIRA (v7.6.14#76016) - To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org