You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Siddharth Ahuja <sa...@cloudera.com> on 2017/06/22 05:48:36 UTC
Review Request 60357: NetcatSource - Socket not closed when an
exception is
encountered during start() leading to file descriptor leaks
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60357/
-----------------------------------------------------------
Review request for Flume and Attila Simon.
Repository: flume-git
Description
-------
Review request for: https://issues.apache.org/jira/browse/FLUME-2905 which is trying to prevent socket leaks when a netcat port is already bound to an existing process.
Diffs
-----
flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java 9513902
flume-ng-core/src/test/java/org/apache/flume/source/TestNetcatSource.java 99d413a
Diff: https://reviews.apache.org/r/60357/diff/1/
Testing
-------
I have tested flume-ng executable generated from my changes and I can confirm from the lsof output that the sockets do not keep increasing if the port to which netcat source is trying to bind to is already in use.
The junits are also passing for me for the NetcatSource.
Thanks,
Siddharth Ahuja
Re: Review Request 60357: NetcatSource - Socket not closed when an
exception
is encountered during start() leading to file descriptor leaks
Posted by Denes Arvay <de...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60357/#review179264
-----------------------------------------------------------
Ship it!
LGTM, I'm about to commit this.
- Denes Arvay
On June 28, 2017, 6:58 a.m., Siddharth Ahuja wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60357/
> -----------------------------------------------------------
>
> (Updated June 28, 2017, 6:58 a.m.)
>
>
> Review request for Flume and Attila Simon.
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Review request for: https://issues.apache.org/jira/browse/FLUME-2905 which is trying to prevent socket leaks when a netcat port is already bound to an existing process.
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java 9513902
> flume-ng-core/src/test/java/org/apache/flume/source/TestNetcatSource.java 99d413a
>
>
> Diff: https://reviews.apache.org/r/60357/diff/2/
>
>
> Testing
> -------
>
> I have tested flume-ng executable generated from my changes and I can confirm from the lsof output that the sockets do not keep increasing if the port to which netcat source is trying to bind to is already in use.
> The junits are also passing for me for the NetcatSource.
>
>
> Thanks,
>
> Siddharth Ahuja
>
>
Re: Review Request 60357: NetcatSource - Socket not closed when an
exception
is encountered during start() leading to file descriptor leaks
Posted by Attila Simon <sa...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60357/#review179084
-----------------------------------------------------------
Ship it!
Ship It!
- Attila Simon
On June 28, 2017, 4:58 a.m., Siddharth Ahuja wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60357/
> -----------------------------------------------------------
>
> (Updated June 28, 2017, 4:58 a.m.)
>
>
> Review request for Flume and Attila Simon.
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Review request for: https://issues.apache.org/jira/browse/FLUME-2905 which is trying to prevent socket leaks when a netcat port is already bound to an existing process.
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java 9513902
> flume-ng-core/src/test/java/org/apache/flume/source/TestNetcatSource.java 99d413a
>
>
> Diff: https://reviews.apache.org/r/60357/diff/2/
>
>
> Testing
> -------
>
> I have tested flume-ng executable generated from my changes and I can confirm from the lsof output that the sockets do not keep increasing if the port to which netcat source is trying to bind to is already in use.
> The junits are also passing for me for the NetcatSource.
>
>
> Thanks,
>
> Siddharth Ahuja
>
>
Re: Review Request 60357: NetcatSource - Socket not closed when an
exception
is encountered during start() leading to file descriptor leaks
Posted by Siddharth Ahuja <sa...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60357/
-----------------------------------------------------------
(Updated June 28, 2017, 4:58 a.m.)
Review request for Flume and Attila Simon.
Changes
-------
Updated code based on comments from Attila Simon.
Repository: flume-git
Description
-------
Review request for: https://issues.apache.org/jira/browse/FLUME-2905 which is trying to prevent socket leaks when a netcat port is already bound to an existing process.
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java 9513902
flume-ng-core/src/test/java/org/apache/flume/source/TestNetcatSource.java 99d413a
Diff: https://reviews.apache.org/r/60357/diff/2/
Changes: https://reviews.apache.org/r/60357/diff/1-2/
Testing
-------
I have tested flume-ng executable generated from my changes and I can confirm from the lsof output that the sockets do not keep increasing if the port to which netcat source is trying to bind to is already in use.
The junits are also passing for me for the NetcatSource.
Thanks,
Siddharth Ahuja
Re: Review Request 60357: NetcatSource - Socket not closed when an
exception
is encountered during start() leading to file descriptor leaks
Posted by Attila Simon <sa...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60357/#review178679
-----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java
Lines 176 (patched)
<https://reviews.apache.org/r/60357/#comment252825>
Please remove the unneccessary whitespaces (highlighted in red). Here and in the test class.
flume-ng-core/src/test/java/org/apache/flume/source/TestNetcatSource.java
Lines 322-324 (patched)
<https://reviews.apache.org/r/60357/#comment252849>
Nit: You can merge line #322 and #324 and use try-with-resources
```
try (ServerSocketChannel dummyServerSocket = ServerSocketChannel.open()) {
...
}
```
and remove the dummyServerSocket.close() from the end
flume-ng-core/src/test/java/org/apache/flume/source/TestNetcatSource.java
Lines 330 (patched)
<https://reviews.apache.org/r/60357/#comment252848>
I think you should avoid using the startSource() for this test as it would be responsible for selecting a free port (even though it is buggy and doesn't do that). This would be the replacement:
```
Context context = new Context();
context.put("port", String.valueOf(PORT));
context.put("bind", "0.0.0.0");
context.put("ack-every-event", "false");
Configurables.configure(source, context);
source.start();
```
- Attila Simon
On June 22, 2017, 5:48 a.m., Siddharth Ahuja wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60357/
> -----------------------------------------------------------
>
> (Updated June 22, 2017, 5:48 a.m.)
>
>
> Review request for Flume and Attila Simon.
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Review request for: https://issues.apache.org/jira/browse/FLUME-2905 which is trying to prevent socket leaks when a netcat port is already bound to an existing process.
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java 9513902
> flume-ng-core/src/test/java/org/apache/flume/source/TestNetcatSource.java 99d413a
>
>
> Diff: https://reviews.apache.org/r/60357/diff/1/
>
>
> Testing
> -------
>
> I have tested flume-ng executable generated from my changes and I can confirm from the lsof output that the sockets do not keep increasing if the port to which netcat source is trying to bind to is already in use.
> The junits are also passing for me for the NetcatSource.
>
>
> Thanks,
>
> Siddharth Ahuja
>
>