You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Juhani Connolly <ju...@gmail.com> on 2012/03/19 04:59:01 UTC
Review Request: Flume-1026: Document thread safety expectations of interfaces
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4400/
-----------------------------------------------------------
Review request for Flume.
Summary
-------
I went over the basic interfaces and added some javadoc for developers of new components.
I am pretty concerned about the fact that pretty much everything is not thread safe because off the lack of guarantees provided by some of the interfaces(Configurable in particular).
A lot of existing components assume thread safety(which is somewhat given by isolation of process() type methods to a single runner thread). Should we be fixing these components, or making some more guarantees about when and how configuration calls should be called(e.g. only callable on a stopped component?)
This addresses bug Flume-1026.
https://issues.apache.org/jira/browse/Flume-1026
Diffs
-----
flume-ng-core/src/main/java/org/apache/flume/Channel.java 9d1c14c
flume-ng-core/src/main/java/org/apache/flume/PollableSource.java 8806040
flume-ng-core/src/main/java/org/apache/flume/Sink.java 4a706cf
flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java ea6000b
flume-ng-core/src/main/java/org/apache/flume/Source.java f4e9481
flume-ng-core/src/main/java/org/apache/flume/Transaction.java 5b1fee5
flume-ng-core/src/main/java/org/apache/flume/conf/Configurable.java 0fa4839
Diff: https://reviews.apache.org/r/4400/diff
Testing
-------
Doc only patch
Thanks,
Juhani
Re: Review Request: Flume-1026: Document thread safety expectations of
interfaces
Posted by Mike Percy <mp...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4400/#review6153
-----------------------------------------------------------
Juhani, looks great. I added a few suggestions around wording.
I agree with the general concerns. I'm not yet sure what I think we should do about it though.
flume-ng-core/src/main/java/org/apache/flume/Channel.java
<https://reviews.apache.org/r/4400/#comment13238>
"...they may be simultaneously accessed by" (missing "by")
flume-ng-core/src/main/java/org/apache/flume/PollableSource.java
<https://reviews.apache.org/r/4400/#comment13239>
How about:
"guaranteed to be called only by a single thread at a time, with no concurrency. Any other mechanism driving a pollable source must follow the same semantics."
This sounds a lot like a synchronized call and I wonder if it makes sense to call this out as required by PollableSource implementations.
flume-ng-core/src/main/java/org/apache/flume/Sink.java
<https://reviews.apache.org/r/4400/#comment13242>
How about switching a couple words around:
"While the Sink process call is guaranteed to only be accessed by a single thread, other calls may be concurrently accessed and should thus be protected."
- Mike
On 2012-03-19 03:59:00, Juhani Connolly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4400/
> -----------------------------------------------------------
>
> (Updated 2012-03-19 03:59:00)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> I went over the basic interfaces and added some javadoc for developers of new components.
>
> I am pretty concerned about the fact that pretty much everything is not thread safe because off the lack of guarantees provided by some of the interfaces(Configurable in particular).
>
> A lot of existing components assume thread safety(which is somewhat given by isolation of process() type methods to a single runner thread). Should we be fixing these components, or making some more guarantees about when and how configuration calls should be called(e.g. only callable on a stopped component?)
>
>
> This addresses bug Flume-1026.
> https://issues.apache.org/jira/browse/Flume-1026
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/Channel.java 9d1c14c
> flume-ng-core/src/main/java/org/apache/flume/PollableSource.java 8806040
> flume-ng-core/src/main/java/org/apache/flume/Sink.java 4a706cf
> flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java ea6000b
> flume-ng-core/src/main/java/org/apache/flume/Source.java f4e9481
> flume-ng-core/src/main/java/org/apache/flume/Transaction.java 5b1fee5
> flume-ng-core/src/main/java/org/apache/flume/conf/Configurable.java 0fa4839
>
> Diff: https://reviews.apache.org/r/4400/diff
>
>
> Testing
> -------
>
> Doc only patch
>
>
> Thanks,
>
> Juhani
>
>
Re: Review Request: Flume-1026: Document thread safety expectations of
interfaces
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4400/#review6211
-----------------------------------------------------------
Ship it!
+1
- Arvind
On 2012-03-22 04:00:33, Juhani Connolly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4400/
> -----------------------------------------------------------
>
> (Updated 2012-03-22 04:00:33)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> I went over the basic interfaces and added some javadoc for developers of new components.
>
> I am pretty concerned about the fact that pretty much everything is not thread safe because off the lack of guarantees provided by some of the interfaces(Configurable in particular).
>
> A lot of existing components assume thread safety(which is somewhat given by isolation of process() type methods to a single runner thread). Should we be fixing these components, or making some more guarantees about when and how configuration calls should be called(e.g. only callable on a stopped component?)
>
>
> This addresses bug Flume-1026.
> https://issues.apache.org/jira/browse/Flume-1026
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/Channel.java 9d1c14c
> flume-ng-core/src/main/java/org/apache/flume/PollableSource.java 8806040
> flume-ng-core/src/main/java/org/apache/flume/Sink.java 4a706cf
> flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java ea6000b
> flume-ng-core/src/main/java/org/apache/flume/Source.java f4e9481
> flume-ng-core/src/main/java/org/apache/flume/Transaction.java 5b1fee5
> flume-ng-core/src/main/java/org/apache/flume/conf/Configurable.java 0fa4839
>
> Diff: https://reviews.apache.org/r/4400/diff
>
>
> Testing
> -------
>
> Doc only patch
>
>
> Thanks,
>
> Juhani
>
>
Re: Review Request: Flume-1026: Document thread safety expectations of
interfaces
Posted by Mike Percy <mp...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4400/#review6210
-----------------------------------------------------------
+1 from me, thanks for writing this up and we will need to resolve this pretty soon
- Mike
On 2012-03-22 04:00:33, Juhani Connolly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4400/
> -----------------------------------------------------------
>
> (Updated 2012-03-22 04:00:33)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> I went over the basic interfaces and added some javadoc for developers of new components.
>
> I am pretty concerned about the fact that pretty much everything is not thread safe because off the lack of guarantees provided by some of the interfaces(Configurable in particular).
>
> A lot of existing components assume thread safety(which is somewhat given by isolation of process() type methods to a single runner thread). Should we be fixing these components, or making some more guarantees about when and how configuration calls should be called(e.g. only callable on a stopped component?)
>
>
> This addresses bug Flume-1026.
> https://issues.apache.org/jira/browse/Flume-1026
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/Channel.java 9d1c14c
> flume-ng-core/src/main/java/org/apache/flume/PollableSource.java 8806040
> flume-ng-core/src/main/java/org/apache/flume/Sink.java 4a706cf
> flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java ea6000b
> flume-ng-core/src/main/java/org/apache/flume/Source.java f4e9481
> flume-ng-core/src/main/java/org/apache/flume/Transaction.java 5b1fee5
> flume-ng-core/src/main/java/org/apache/flume/conf/Configurable.java 0fa4839
>
> Diff: https://reviews.apache.org/r/4400/diff
>
>
> Testing
> -------
>
> Doc only patch
>
>
> Thanks,
>
> Juhani
>
>
Re: Review Request: Flume-1026: Document thread safety expectations of
interfaces
Posted by Juhani Connolly <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4400/
-----------------------------------------------------------
(Updated 2012-03-22 04:00:33.408798)
Review request for Flume.
Changes
-------
Added in Mike's wording suggestions.
Will: yeah, I think that configuration is at the center of the whole threading issue and we need to address that before we come around to individual components.
Summary
-------
I went over the basic interfaces and added some javadoc for developers of new components.
I am pretty concerned about the fact that pretty much everything is not thread safe because off the lack of guarantees provided by some of the interfaces(Configurable in particular).
A lot of existing components assume thread safety(which is somewhat given by isolation of process() type methods to a single runner thread). Should we be fixing these components, or making some more guarantees about when and how configuration calls should be called(e.g. only callable on a stopped component?)
This addresses bug Flume-1026.
https://issues.apache.org/jira/browse/Flume-1026
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/Channel.java 9d1c14c
flume-ng-core/src/main/java/org/apache/flume/PollableSource.java 8806040
flume-ng-core/src/main/java/org/apache/flume/Sink.java 4a706cf
flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java ea6000b
flume-ng-core/src/main/java/org/apache/flume/Source.java f4e9481
flume-ng-core/src/main/java/org/apache/flume/Transaction.java 5b1fee5
flume-ng-core/src/main/java/org/apache/flume/conf/Configurable.java 0fa4839
Diff: https://reviews.apache.org/r/4400/diff
Testing
-------
Doc only patch
Thanks,
Juhani
Re: Review Request: Flume-1026: Document thread safety expectations of
interfaces
Posted by Will McQueen <wi...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4400/#review6156
-----------------------------------------------------------
I too am concerned about the thread safety. I'll take a look at some of the classes tonight or tomorrow, and will offer some suggestions. I'll be considering atomicity, visibility, and HB relationships per JCIP.
- Will
On 2012-03-19 03:59:00, Juhani Connolly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4400/
> -----------------------------------------------------------
>
> (Updated 2012-03-19 03:59:00)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> I went over the basic interfaces and added some javadoc for developers of new components.
>
> I am pretty concerned about the fact that pretty much everything is not thread safe because off the lack of guarantees provided by some of the interfaces(Configurable in particular).
>
> A lot of existing components assume thread safety(which is somewhat given by isolation of process() type methods to a single runner thread). Should we be fixing these components, or making some more guarantees about when and how configuration calls should be called(e.g. only callable on a stopped component?)
>
>
> This addresses bug Flume-1026.
> https://issues.apache.org/jira/browse/Flume-1026
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/Channel.java 9d1c14c
> flume-ng-core/src/main/java/org/apache/flume/PollableSource.java 8806040
> flume-ng-core/src/main/java/org/apache/flume/Sink.java 4a706cf
> flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java ea6000b
> flume-ng-core/src/main/java/org/apache/flume/Source.java f4e9481
> flume-ng-core/src/main/java/org/apache/flume/Transaction.java 5b1fee5
> flume-ng-core/src/main/java/org/apache/flume/conf/Configurable.java 0fa4839
>
> Diff: https://reviews.apache.org/r/4400/diff
>
>
> Testing
> -------
>
> Doc only patch
>
>
> Thanks,
>
> Juhani
>
>