You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Balázs Donát Bessenyei <be...@cloudera.com> on 2016/07/20 13:25:24 UTC
Review Request 50232: FLUME-2619: Spooldir source does not log channel
exceptions
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50232/
-----------------------------------------------------------
Review request for Flume, Denes Arvay and Attila Simon.
Repository: flume-git
Description
-------
Spooldir assumes that any ChannelException means that the channel is full and it does not log the exception message.
Diffs
-----
flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java 1cce137
flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java d88cc1d
flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java 82c5351
Diff: https://reviews.apache.org/r/50232/diff/
Testing
-------
[INFO] Flume NG Core ...................................... SUCCESS [08:04 min]
Thanks,
Bal�zs Don�t Bessenyei
Re: Review Request 50232: FLUME-2619: Spooldir source does not log
channel exceptions
Posted by Denes Arvay <de...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50232/#review143088
-----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java (line 211)
<https://reviews.apache.org/r/50232/#comment208797>
nit (and I know that it was the same before): as a getter for a boolean value it'd be better to start with `is` (or if it sounds better `get`) prefix.
flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java (line 250)
<https://reviews.apache.org/r/50232/#comment208796>
My concern with this change is that if a `ChannelException` is thrown then it'll end up in the outer `catch (Throwable t)` (line 267) block and it will be considered as fatal error which isn't the case currently.
As far as I understood the Jira issue the main goal is to be able to make distinction between `ChannelFullException` and other `ChannelException`s and in the latter case log the original exception but the current behaviour shouldn't be changed.
- Denes Arvay
On July 20, 2016, 1:25 p.m., Bal�zs Don�t Bessenyei wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50232/
> -----------------------------------------------------------
>
> (Updated July 20, 2016, 1:25 p.m.)
>
>
> Review request for Flume, Denes Arvay and Attila Simon.
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Spooldir assumes that any ChannelException means that the channel is full and it does not log the exception message.
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java 1cce137
> flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java d88cc1d
> flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java 82c5351
>
> Diff: https://reviews.apache.org/r/50232/diff/
>
>
> Testing
> -------
>
> [INFO] Flume NG Core ...................................... SUCCESS [08:04 min]
>
>
> Thanks,
>
> Bal�zs Don�t Bessenyei
>
>
Re: Review Request 50232: FLUME-2619: Spooldir source does not log
channel exceptions
Posted by Denes Arvay <de...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50232/#review145074
-----------------------------------------------------------
Fix it, then Ship it!
One comment otherwise LGTM
flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java (line 285)
<https://reviews.apache.org/r/50232/#comment211257>
please choose a more descriptive name for this, e.g. `waitAndGetNewBackoffInterval`, or similar.
- Denes Arvay
On Aug. 3, 2016, 3:54 p.m., Bal�zs Don�t Bessenyei wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50232/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2016, 3:54 p.m.)
>
>
> Review request for Flume, Denes Arvay and Attila Simon.
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Spooldir assumes that any ChannelException means that the channel is full and it does not log the exception message.
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java d88cc1d
> flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java 82c5351
>
> Diff: https://reviews.apache.org/r/50232/diff/
>
>
> Testing
> -------
>
> [INFO] Flume NG Core ...................................... SUCCESS [08:04 min]
>
>
> Thanks,
>
> Bal�zs Don�t Bessenyei
>
>
Re: Review Request 50232: FLUME-2619: Spooldir source does not log
channel exceptions
Posted by Mike Percy <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50232/#review145021
-----------------------------------------------------------
Overall looks good.
flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java (line 213)
<https://reviews.apache.org/r/50232/#comment211176>
nit: Would you mind renaming this to didHitChannelException() ? I think it's more descriptive
flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java (line 218)
<https://reviews.apache.org/r/50232/#comment211177>
nit: How about didHitChannelFullException() ?
flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 388)
<https://reviews.apache.org/r/50232/#comment211178>
nit: please either use hitChannelFullException here or don't save it above and just call source.getHitChannelFullException() in the while loop. Otherwise there's really no benefit to storing the variable.
- Mike Percy
On Aug. 3, 2016, 8:54 a.m., Bal�zs Don�t Bessenyei wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50232/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2016, 8:54 a.m.)
>
>
> Review request for Flume, Denes Arvay and Attila Simon.
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Spooldir assumes that any ChannelException means that the channel is full and it does not log the exception message.
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java d88cc1d
> flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java 82c5351
>
> Diff: https://reviews.apache.org/r/50232/diff/
>
>
> Testing
> -------
>
> [INFO] Flume NG Core ...................................... SUCCESS [08:04 min]
>
>
> Thanks,
>
> Bal�zs Don�t Bessenyei
>
>
Re: Review Request 50232: FLUME-2619: Spooldir source does not log
channel exceptions
Posted by Mike Percy <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50232/#review145158
-----------------------------------------------------------
Ship it!
Ship It!
- Mike Percy
On Aug. 8, 2016, 8:23 a.m., Bal�zs Don�t Bessenyei wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50232/
> -----------------------------------------------------------
>
> (Updated Aug. 8, 2016, 8:23 a.m.)
>
>
> Review request for Flume, Denes Arvay and Attila Simon.
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Spooldir assumes that any ChannelException means that the channel is full and it does not log the exception message.
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java 1cce137
> flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java d88cc1d
> flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java 82c5351
>
> Diff: https://reviews.apache.org/r/50232/diff/
>
>
> Testing
> -------
>
> [INFO] Flume NG Core ...................................... SUCCESS [08:04 min]
>
>
> Thanks,
>
> Bal�zs Don�t Bessenyei
>
>
Re: Review Request 50232: FLUME-2619: Spooldir source does not log
channel exceptions
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/50232/
-----------------------------------------------------------
(Updated Aug. 8, 2016, 3:23 p.m.)
Review request for Flume, Denes Arvay and Attila Simon.
Repository: flume-git
Description
-------
Spooldir assumes that any ChannelException means that the channel is full and it does not log the exception message.
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java 1cce137
flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java d88cc1d
flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java 82c5351
Diff: https://reviews.apache.org/r/50232/diff/
Testing
-------
[INFO] Flume NG Core ...................................... SUCCESS [08:04 min]
Thanks,
Bal�zs Don�t Bessenyei
Re: Review Request 50232: FLUME-2619: Spooldir source does not log
channel exceptions
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/50232/
-----------------------------------------------------------
(Updated Aug. 3, 2016, 3:54 p.m.)
Review request for Flume, Denes Arvay and Attila Simon.
Repository: flume-git
Description
-------
Spooldir assumes that any ChannelException means that the channel is full and it does not log the exception message.
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java d88cc1d
flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java 82c5351
Diff: https://reviews.apache.org/r/50232/diff/
Testing
-------
[INFO] Flume NG Core ...................................... SUCCESS [08:04 min]
Thanks,
Bal�zs Don�t Bessenyei
Re: Review Request 50232: FLUME-2619: Spooldir source does not log
channel exceptions
Posted by Denes Arvay <de...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50232/#review144556
-----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java (lines 257 - 281)
<https://reviews.apache.org/r/50232/#comment210586>
could you please somehow eliminate the copy-paste here? Either by extracting the `if (backoff) {...}` block to a method or by combining the two `catch` blocks to one `catch (ChannelException)` and doing `instanceof` checks inside it. (I'd prefer the former one)
flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java (line 259)
<https://reviews.apache.org/r/50232/#comment210587>
Minor: `String.valueOf` is unnecessary here and it's easier to read without it.
flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java (line 402)
<https://reviews.apache.org/r/50232/#comment210589>
This check cannot fail because the `while` loop in line 308 makes sure that `hitChannelFullException` is true.
- Denes Arvay
On July 22, 2016, 1:33 p.m., Bal�zs Don�t Bessenyei wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50232/
> -----------------------------------------------------------
>
> (Updated July 22, 2016, 1:33 p.m.)
>
>
> Review request for Flume, Denes Arvay and Attila Simon.
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Spooldir assumes that any ChannelException means that the channel is full and it does not log the exception message.
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java 1cce137
> flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java d88cc1d
> flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java 82c5351
>
> Diff: https://reviews.apache.org/r/50232/diff/
>
>
> Testing
> -------
>
> [INFO] Flume NG Core ...................................... SUCCESS [08:04 min]
>
>
> Thanks,
>
> Bal�zs Don�t Bessenyei
>
>
Re: Review Request 50232: FLUME-2619: Spooldir source does not log
channel exceptions
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/50232/
-----------------------------------------------------------
(Updated July 22, 2016, 1:33 p.m.)
Review request for Flume, Denes Arvay and Attila Simon.
Repository: flume-git
Description
-------
Spooldir assumes that any ChannelException means that the channel is full and it does not log the exception message.
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java 1cce137
flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java d88cc1d
flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java 82c5351
Diff: https://reviews.apache.org/r/50232/diff/
Testing
-------
[INFO] Flume NG Core ...................................... SUCCESS [08:04 min]
Thanks,
Bal�zs Don�t Bessenyei
Re: Review Request 50232: FLUME-2619: Spooldir source does not log
channel exceptions
Posted by Alexander Alten-Lorenz <al...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50232/#review143058
-----------------------------------------------------------
Ship it!
- Alexander Alten-Lorenz
On July 20, 2016, 1:25 p.m., Bal�zs Don�t Bessenyei wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50232/
> -----------------------------------------------------------
>
> (Updated July 20, 2016, 1:25 p.m.)
>
>
> Review request for Flume, Denes Arvay and Attila Simon.
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Spooldir assumes that any ChannelException means that the channel is full and it does not log the exception message.
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/channel/ChannelProcessor.java 1cce137
> flume-ng-core/src/main/java/org/apache/flume/source/SpoolDirectorySource.java d88cc1d
> flume-ng-core/src/test/java/org/apache/flume/source/TestSpoolDirectorySource.java 82c5351
>
> Diff: https://reviews.apache.org/r/50232/diff/
>
>
> Testing
> -------
>
> [INFO] Flume NG Core ...................................... SUCCESS [08:04 min]
>
>
> Thanks,
>
> Bal�zs Don�t Bessenyei
>
>