You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Denes Arvay <de...@cloudera.com> on 2016/07/28 11:33:26 UTC
Review Request 50562: Patch for FLUME-2797 - SyslogTcpSource uses
Deprecated Class + Deprecate Syslog Source
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50562/
-----------------------------------------------------------
Review request for Flume, Bal�zs Don�t Bessenyei, Mike Percy, and Attila Simon.
Summary (updated)
-----------------
Patch for FLUME-2797 - SyslogTcpSource uses Deprecated Class + Deprecate Syslog Source
Bugs: FLUME-2797
https://issues.apache.org/jira/browse/FLUME-2797
Repository: flume-git
Description (updated)
-------
Patch for FLUME-2797 - SyslogTcpSource uses Deprecated Class + Deprecate SyslogTCPSource
Diffs (updated)
-----
flume-ng-channels/flume-spillable-memory-channel/src/main/java/org/apache/flume/channel/SpillableMemoryChannel.java b46d646
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 185c00c
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 175bebb
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 10ef8d8
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java e5b7a06
Diff: https://reviews.apache.org/r/50562/diff/
Testing (updated)
-------
- checkstyle passes
- `flume-ng-core` tests pass
- 2 new tests added
Thanks,
Denes Arvay
Re: Review Request 50562: Patch for FLUME-2797 - SyslogTcpSource uses
Deprecated Class + Deprecate Syslog Source
Posted by Mike Percy <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50562/#review147234
-----------------------------------------------------------
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java (line 205)
<https://reviews.apache.org/r/50562/#comment214350>
Prefer try-with-resources here:
try (DatagramSocket syslogSocket = new DatagramSocket()) {
syslogSocket.send(datagramPacket);
}
See https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
- Mike Percy
On Aug. 18, 2016, 6:44 a.m., Denes Arvay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50562/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2016, 6:44 a.m.)
>
>
> Review request for Flume, Bal�zs Don�t Bessenyei, Mike Percy, and Attila Simon.
>
>
> Bugs: FLUME-2797
> https://issues.apache.org/jira/browse/FLUME-2797
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Patch for FLUME-2797 - SyslogTcpSource uses Deprecated Class + Deprecate SyslogTCPSource
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 185c00c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 175bebb
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 10ef8d8
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java e5b7a06
>
> Diff: https://reviews.apache.org/r/50562/diff/
>
>
> Testing
> -------
>
> - checkstyle passes
> - `flume-ng-core` tests pass
> - 2 new tests added
>
>
> Thanks,
>
> Denes Arvay
>
>
Re: Review Request 50562: Patch for FLUME-2797 - SyslogTcpSource uses
Deprecated Class + Deprecate Syslog Source
Posted by Mike Percy <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50562/#review147228
-----------------------------------------------------------
This is not your fault however let's clean up this test a little to make it less flaky.
Could you please do this for both the TCP and the UDP sources?
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java (line 201)
<https://reviews.apache.org/r/50562/#comment214343>
Prefer InetAddress.getLoopbackAddress(), which does not rely on the machine having a DNS entry and will not throw UnknownHostException.
However... even better than this would be to expose a method getBoundAddress() on the source itself that returns the bind address, similar to how it's done for the port. In fact, you can just replace the port one with the bound address one. Something like this:
@VisibleForTesting
public InetSocketAddress getBoundAddress() {
SocketAddress localAddress = nettyChannel.getLocalAddress();
if (!(localAddress instanceof InetSocketAddress)) {
throw IllegalArgumentException("Not bound to an internet address");
}
return (InetSocketAddress) localAddress;
}
Then use:
InetAddress addr = source.getBoundAddress();
return new DatagramPacket(payload, payload.length, boundAddr.getAddress(), boundAddr.getPort());
- Mike Percy
On Aug. 18, 2016, 6:44 a.m., Denes Arvay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50562/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2016, 6:44 a.m.)
>
>
> Review request for Flume, Bal�zs Don�t Bessenyei, Mike Percy, and Attila Simon.
>
>
> Bugs: FLUME-2797
> https://issues.apache.org/jira/browse/FLUME-2797
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Patch for FLUME-2797 - SyslogTcpSource uses Deprecated Class + Deprecate SyslogTCPSource
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 185c00c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 175bebb
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 10ef8d8
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java e5b7a06
>
> Diff: https://reviews.apache.org/r/50562/diff/
>
>
> Testing
> -------
>
> - checkstyle passes
> - `flume-ng-core` tests pass
> - 2 new tests added
>
>
> Thanks,
>
> Denes Arvay
>
>
Re: Review Request 50562: Patch for FLUME-2797 - SyslogTcpSource uses
Deprecated Class + Deprecate Syslog Source
Posted by Mike Percy <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50562/#review148207
-----------------------------------------------------------
Ship it!
Ship It!
- Mike Percy
On Sept. 2, 2016, 5:48 a.m., Denes Arvay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50562/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2016, 5:48 a.m.)
>
>
> Review request for Flume, Bal�zs Don�t Bessenyei, Mike Percy, and Attila Simon.
>
>
> Bugs: FLUME-2797
> https://issues.apache.org/jira/browse/FLUME-2797
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Patch for FLUME-2797 - SyslogTcpSource uses Deprecated Class + Deprecate SyslogTCPSource
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 185c00c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 175bebb
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 10ef8d8
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java e5b7a06
>
> Diff: https://reviews.apache.org/r/50562/diff/
>
>
> Testing
> -------
>
> - checkstyle passes
> - `flume-ng-core` tests pass
> - 2 new tests added
>
>
> Thanks,
>
> Denes Arvay
>
>
Re: Review Request 50562: Patch for FLUME-2797 - SyslogTcpSource uses
Deprecated Class + Deprecate Syslog Source
Posted by Balázs Donát Bessenyei <be...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50562/#review147653
-----------------------------------------------------------
Ship it!
Ship It!
- Bal�zs Don�t Bessenyei
On Sept. 2, 2016, 12:48 p.m., Denes Arvay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50562/
> -----------------------------------------------------------
>
> (Updated Sept. 2, 2016, 12:48 p.m.)
>
>
> Review request for Flume, Bal�zs Don�t Bessenyei, Mike Percy, and Attila Simon.
>
>
> Bugs: FLUME-2797
> https://issues.apache.org/jira/browse/FLUME-2797
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Patch for FLUME-2797 - SyslogTcpSource uses Deprecated Class + Deprecate SyslogTCPSource
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 185c00c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 175bebb
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 10ef8d8
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java e5b7a06
>
> Diff: https://reviews.apache.org/r/50562/diff/
>
>
> Testing
> -------
>
> - checkstyle passes
> - `flume-ng-core` tests pass
> - 2 new tests added
>
>
> Thanks,
>
> Denes Arvay
>
>
Re: Review Request 50562: Patch for FLUME-2797 - SyslogTcpSource uses
Deprecated Class + Deprecate Syslog Source
Posted by Denes Arvay <de...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50562/
-----------------------------------------------------------
(Updated Sept. 2, 2016, 12:48 p.m.)
Review request for Flume, Bal�zs Don�t Bessenyei, Mike Percy, and Attila Simon.
Changes
-------
updated diff according to Mike's review
Bugs: FLUME-2797
https://issues.apache.org/jira/browse/FLUME-2797
Repository: flume-git
Description
-------
Patch for FLUME-2797 - SyslogTcpSource uses Deprecated Class + Deprecate SyslogTCPSource
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 185c00c
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 175bebb
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 10ef8d8
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java e5b7a06
Diff: https://reviews.apache.org/r/50562/diff/
Testing
-------
- checkstyle passes
- `flume-ng-core` tests pass
- 2 new tests added
Thanks,
Denes Arvay
Re: Review Request 50562: Patch for FLUME-2797 - SyslogTcpSource uses
Deprecated Class + Deprecate Syslog Source
Posted by Lior Zeno <li...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50562/#review146088
-----------------------------------------------------------
Ship it!
Ship It!
- Lior Zeno
On Aug. 18, 2016, 1:44 p.m., Denes Arvay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50562/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2016, 1:44 p.m.)
>
>
> Review request for Flume, Bal�zs Don�t Bessenyei, Mike Percy, and Attila Simon.
>
>
> Bugs: FLUME-2797
> https://issues.apache.org/jira/browse/FLUME-2797
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Patch for FLUME-2797 - SyslogTcpSource uses Deprecated Class + Deprecate SyslogTCPSource
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 185c00c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 175bebb
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 10ef8d8
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java e5b7a06
>
> Diff: https://reviews.apache.org/r/50562/diff/
>
>
> Testing
> -------
>
> - checkstyle passes
> - `flume-ng-core` tests pass
> - 2 new tests added
>
>
> Thanks,
>
> Denes Arvay
>
>
Re: Review Request 50562: Patch for FLUME-2797 - SyslogTcpSource uses
Deprecated Class + Deprecate Syslog Source
Posted by Mike Percy <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50562/#review147233
-----------------------------------------------------------
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java (line 88)
<https://reviews.apache.org/r/50562/#comment214349>
Please implement the suggestion for the UDP source test here as well. Let's get the bound address and port from the source's bound socket rather than try to use DNS to figure it out.
- Mike Percy
On Aug. 18, 2016, 6:44 a.m., Denes Arvay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50562/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2016, 6:44 a.m.)
>
>
> Review request for Flume, Bal�zs Don�t Bessenyei, Mike Percy, and Attila Simon.
>
>
> Bugs: FLUME-2797
> https://issues.apache.org/jira/browse/FLUME-2797
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Patch for FLUME-2797 - SyslogTcpSource uses Deprecated Class + Deprecate SyslogTCPSource
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 185c00c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 175bebb
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 10ef8d8
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java e5b7a06
>
> Diff: https://reviews.apache.org/r/50562/diff/
>
>
> Testing
> -------
>
> - checkstyle passes
> - `flume-ng-core` tests pass
> - 2 new tests added
>
>
> Thanks,
>
> Denes Arvay
>
>
Re: Review Request 50562: Patch for FLUME-2797 - SyslogTcpSource uses
Deprecated Class + Deprecate Syslog Source
Posted by Denes Arvay <de...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50562/
-----------------------------------------------------------
(Updated Aug. 18, 2016, 1:44 p.m.)
Review request for Flume, Bal�zs Don�t Bessenyei, Mike Percy, and Attila Simon.
Changes
-------
Add logging when `txn.commit()` throws an exception
Bugs: FLUME-2797
https://issues.apache.org/jira/browse/FLUME-2797
Repository: flume-git
Description
-------
Patch for FLUME-2797 - SyslogTcpSource uses Deprecated Class + Deprecate SyslogTCPSource
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 185c00c
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 175bebb
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 10ef8d8
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java e5b7a06
Diff: https://reviews.apache.org/r/50562/diff/
Testing
-------
- checkstyle passes
- `flume-ng-core` tests pass
- 2 new tests added
Thanks,
Denes Arvay
Re: Review Request 50562: Patch for FLUME-2797 - SyslogTcpSource uses
Deprecated Class + Deprecate Syslog Source
Posted by Balázs Donát Bessenyei <be...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50562/#review145325
-----------------------------------------------------------
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java (line 214)
<https://reviews.apache.org/r/50562/#comment211516>
I think, some logging could be useful here.
- Bal�zs Don�t Bessenyei
On July 29, 2016, 3:10 p.m., Denes Arvay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50562/
> -----------------------------------------------------------
>
> (Updated July 29, 2016, 3:10 p.m.)
>
>
> Review request for Flume, Bal�zs Don�t Bessenyei, Mike Percy, and Attila Simon.
>
>
> Bugs: FLUME-2797
> https://issues.apache.org/jira/browse/FLUME-2797
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Patch for FLUME-2797 - SyslogTcpSource uses Deprecated Class + Deprecate SyslogTCPSource
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 185c00c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 175bebb
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 10ef8d8
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java e5b7a06
>
> Diff: https://reviews.apache.org/r/50562/diff/
>
>
> Testing
> -------
>
> - checkstyle passes
> - `flume-ng-core` tests pass
> - 2 new tests added
>
>
> Thanks,
>
> Denes Arvay
>
>
Re: Review Request 50562: Patch for FLUME-2797 - SyslogTcpSource uses
Deprecated Class + Deprecate Syslog Source
Posted by Denes Arvay <de...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50562/
-----------------------------------------------------------
(Updated July 29, 2016, 3:10 p.m.)
Review request for Flume, Bal�zs Don�t Bessenyei, Mike Percy, and Attila Simon.
Changes
-------
test logging statement removed
Bugs: FLUME-2797
https://issues.apache.org/jira/browse/FLUME-2797
Repository: flume-git
Description
-------
Patch for FLUME-2797 - SyslogTcpSource uses Deprecated Class + Deprecate SyslogTCPSource
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 185c00c
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 175bebb
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 10ef8d8
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java e5b7a06
Diff: https://reviews.apache.org/r/50562/diff/
Testing
-------
- checkstyle passes
- `flume-ng-core` tests pass
- 2 new tests added
Thanks,
Denes Arvay
Re: Review Request 50562: Patch for FLUME-2797 - SyslogTcpSource uses
Deprecated Class + Deprecate Syslog Source
Posted by Attila Simon <sa...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50562/#review144100
-----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java (line 82)
<https://reviews.apache.org/r/50562/#comment210111>
please remove test logging statement
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java (lines 89 - 103)
<https://reviews.apache.org/r/50562/#comment210112>
looks good
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java (lines 131 - 146)
<https://reviews.apache.org/r/50562/#comment210113>
looks good
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java (lines 181 - 219)
<https://reviews.apache.org/r/50562/#comment210114>
looks good
generally looks good, I vote for shipping it after leftover log statement was removed
- Attila Simon
On July 28, 2016, 11:46 a.m., Denes Arvay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50562/
> -----------------------------------------------------------
>
> (Updated July 28, 2016, 11:46 a.m.)
>
>
> Review request for Flume, Bal�zs Don�t Bessenyei, Mike Percy, and Attila Simon.
>
>
> Bugs: FLUME-2797
> https://issues.apache.org/jira/browse/FLUME-2797
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Patch for FLUME-2797 - SyslogTcpSource uses Deprecated Class + Deprecate SyslogTCPSource
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 185c00c
> flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 175bebb
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 10ef8d8
> flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java e5b7a06
>
> Diff: https://reviews.apache.org/r/50562/diff/
>
>
> Testing
> -------
>
> - checkstyle passes
> - `flume-ng-core` tests pass
> - 2 new tests added
>
>
> Thanks,
>
> Denes Arvay
>
>
Re: Review Request 50562: Patch for FLUME-2797 - SyslogTcpSource uses
Deprecated Class + Deprecate Syslog Source
Posted by Denes Arvay <de...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50562/
-----------------------------------------------------------
(Updated July 28, 2016, 11:46 a.m.)
Review request for Flume, Bal�zs Don�t Bessenyei, Mike Percy, and Attila Simon.
Changes
-------
the first patch unintentionally contains unrelated change
Bugs: FLUME-2797
https://issues.apache.org/jira/browse/FLUME-2797
Repository: flume-git
Description
-------
Patch for FLUME-2797 - SyslogTcpSource uses Deprecated Class + Deprecate SyslogTCPSource
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/source/SyslogTcpSource.java 185c00c
flume-ng-core/src/main/java/org/apache/flume/source/SyslogUDPSource.java 175bebb
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogTcpSource.java 10ef8d8
flume-ng-core/src/test/java/org/apache/flume/source/TestSyslogUdpSource.java e5b7a06
Diff: https://reviews.apache.org/r/50562/diff/
Testing
-------
- checkstyle passes
- `flume-ng-core` tests pass
- 2 new tests added
Thanks,
Denes Arvay