You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by darionyaphet <gi...@git.apache.org> on 2016/06/05 14:32:55 UTC
[GitHub] storm pull request #1462: [STORM-1883] FileReader extends Closeable Interfac...
GitHub user darionyaphet opened a pull request:
https://github.com/apache/storm/pull/1462
[STORM-1883] FileReader extends Closeable Interface
[STORM-1883](https://issues.apache.org/jira/browse/STORM-1883)
use Closeable Interface to decorate FileReader to support close()
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/darionyaphet/storm STORM-1883
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1462.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1462
----
commit ad74ad10012df6999c94652993bcd66e90db53ff
Author: darionyaphet <da...@gmail.com>
Date: 2016-06-05T14:31:29Z
make FileReader extends Closeable Interface
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1462: [STORM-1883] FileReader extends Closeable Interface
Posted by darionyaphet <gi...@git.apache.org>.
Github user darionyaphet commented on the issue:
https://github.com/apache/storm/pull/1462
I think `FileReader` should same with 'java.io. FileReader' . Implement `Closeable` interface also have a better extension structure .
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1462: [STORM-1883] FileReader extends Closeable Interface
Posted by dossett <gi...@git.apache.org>.
Github user dossett commented on the issue:
https://github.com/apache/storm/pull/1462
Are there any functional changes here? I'm all in favor of using standard java interfaces, but I don't see any benefits of `closeable` being used here. Possible changes could include:
- Using the try-with-resources idiom now that FileReader is `closeable` [1]
- Concrete classes no longer have to catch their own `IOException`s since close is declared to throw that [2]
[1] - https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
[2] - https://github.com/darionyaphet/storm/blob/ad74ad10012df6999c94652993bcd66e90db53ff/external/storm-hdfs/src/main/java/org/apache/storm/hdfs/spout/TextFileReader.java#L104
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1462: [STORM-1883] FileReader extends Closeable Interfac...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on a diff in the pull request:
https://github.com/apache/storm/pull/1462#discussion_r70193414
--- Diff: external/storm-hdfs/src/main/java/org/apache/storm/hdfs/spout/FileReader.java ---
@@ -20,10 +20,11 @@
import org.apache.hadoop.fs.Path;
+import java.io.Closeable;
import java.io.IOException;
import java.util.List;
-interface FileReader {
+interface FileReader extends Closeable{
--- End diff --
please add space after the Closeable
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1462: [STORM-1883] FileReader extends Closeable Interface
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1462
I'm also in favor of using standard java interface. Just it seems not have any valuable benefits. I'm fine either change or keep it as is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1462: [STORM-1883] FileReader extends Closeable Interface
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/1462
+1 after addressing the minor comment . @roshannaik can you review as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1462: [STORM-1883] FileReader extends Closeable Interfac...
Posted by darionyaphet <gi...@git.apache.org>.
Github user darionyaphet commented on a diff in the pull request:
https://github.com/apache/storm/pull/1462#discussion_r70212495
--- Diff: external/storm-hdfs/src/main/java/org/apache/storm/hdfs/spout/FileReader.java ---
@@ -20,10 +20,11 @@
import org.apache.hadoop.fs.Path;
+import java.io.Closeable;
import java.io.IOException;
import java.util.List;
-interface FileReader {
+interface FileReader extends Closeable{
--- End diff --
fixed :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---