You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jaikiran Pai <ja...@gmail.com> on 2015/05/29 08:25:01 UTC
Nagging - pending review requests :)
Could someone please look at these few review requests and let me know
if any changes are needed:
https://reviews.apache.org/r/34394/ related to
https://issues.apache.org/jira/browse/KAFKA-1907
https://reviews.apache.org/r/30403/ related to
https://issues.apache.org/jira/browse/KAFKA-1906
There's also this one https://reviews.apache.org/r/34697/ for
https://issues.apache.org/jira/browse/KAFKA-2221 but it's only been up
since a couple of days and is a fairly minor one.
-Jaikiran
Re: Nagging - pending review requests :)
Posted by Joe Stein <jo...@stealth.ly>.
Hey Jai, see below
On Fri, May 29, 2015 at 3:03 AM, Jaikiran Pai <ja...@gmail.com>
wrote:
> Hi Joe,
>
> Comments inline.
>
> On Friday 29 May 2015 12:15 PM, Joe Stein wrote:
>
>> see below
>>
>> On Fri, May 29, 2015 at 2:25 AM, Jaikiran Pai <ja...@gmail.com>
>> wrote:
>>
>> Could someone please look at these few review requests and let me know if
>>> any changes are needed:
>>>
>>> https://reviews.apache.org/r/34394/ related to
>>> https://issues.apache.org/jira/browse/KAFKA-1907
>>>
>>
>> I haven't looked at all the other changes that would be introduced from
>> their release that could break between zk and kafka by introducing a zk
>> client bump. A less ops negative way to deal with this might be to create
>> a
>> plugable interface, then someone can use a patched zkclient if they
>> wanted,
>> or exhibitor, or consul, or akka, etc.
>>
>
>
> The ZkClient has already been bumped to this newer version as part of a
> separate task https://issues.apache.org/jira/browse/KAFKA-2169 and it's
> already in trunk. This change in my review request only passes along an
> (optional) value to the ZkClient constructor that was introduced in that
> newer version.
>
>
I left a comment in the review.
>
>
>>
>> https://reviews.apache.org/r/30403/ related to
>>> https://issues.apache.org/jira/browse/KAFKA-1906
>>>
>>
>> I don't understand the patch and how it would fix the issue. I also don't
>> think necessarily there is an issue. Its a balance from the community
>> having a good out of the box experience vs taking defaults and rushing
>> them
>> into production. No matter what we do we can't stop the latter from
>> happening, which will also cause issues.
>>
>
> The change to use a default directory that's within the Kafka installation
> path rather than /tmp folder (which get erased on restarts) is more from a
> development environment point of view rather than production. As you note,
> production environments will anyway have to deal with setting the right
> configs. From a developer perspective, I like the Kafka logs to survive
> system restarts when I'm working on applications which use Kafka. Of
> course, I can go ahead and change that default value in the
> server.properties on each fresh installation. But personally, I like it
> more if the logs are are stored within the Kafka installation itself so
> that even if I have multiple different versions of Kafka running (for
> different applications) on the same system, the logs are isolated to the
> Kafka installation and don't interfere with each other. We currently have a
> development setup where we have a bunch of VMs with different Kafka
> installations. These VMs are then handed out to developers to work on
> various different applications (which are under development). The first
> thing we currently do is edit the server.properties and update the log path
> (and that's the only change we do for dev). It would be much more easier
> and convenient/manageable if this log directory default to a path within
> the Kafka installation.
Developers like things to work when they try them out too. If there is
another way to have something other than /tmp be the default for log.dirs
and still run kinda everywhere folks want it too then lets discuss that as
a thread separately. If you have a proposal for what that is and how it
work you could submit it to
https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals.
I think most developers that use Kafka use it because they have an eye to
production and they check and change things in the configs like the data
being saved to /tmp. The relative dir is a tad scary especially when you
have log and kafka-logs which is which?
This will also be be _really_ confusing to people imho
-# A comma seperated list of directories under which to store log files
-log.dirs=/tmp/kafka-logs
+# A comma separated list of directories under which to store log files
+#log.dirs=
>
>
>
>>
>>> There's also this one https://reviews.apache.org/r/34697/ for
>>> https://issues.apache.org/jira/browse/KAFKA-2221 but it's only been up
>>> since a couple of days and is a fairly minor one.
>>>
>>
>> Folks should start to transition in 0.8.3 to the new java consumer (which
>> is on trunk). If this fix is so critical we should release it in 0.8.2.2
>> otherwise continue to try to not make changes to the existing scalal
>> consumer.
>>
>>
> Fair enough. It was more to help narrow down the real issues when a
> reconnect happens and isn't that critical. Do you want me to close that
> review request?
Your call. Folks may want to patch the change so knowing what version it is
for in the fix is helpful for them to-do that if they wanted. It is also
one less ticket to look at for folks.
>
>
> -Jaikiran
>
Re: Nagging - pending review requests :)
Posted by Jaikiran Pai <ja...@gmail.com>.
Hi Joe,
Comments inline.
On Friday 29 May 2015 12:15 PM, Joe Stein wrote:
> see below
>
> On Fri, May 29, 2015 at 2:25 AM, Jaikiran Pai <ja...@gmail.com>
> wrote:
>
>> Could someone please look at these few review requests and let me know if
>> any changes are needed:
>>
>> https://reviews.apache.org/r/34394/ related to
>> https://issues.apache.org/jira/browse/KAFKA-1907
>
> I haven't looked at all the other changes that would be introduced from
> their release that could break between zk and kafka by introducing a zk
> client bump. A less ops negative way to deal with this might be to create a
> plugable interface, then someone can use a patched zkclient if they wanted,
> or exhibitor, or consul, or akka, etc.
The ZkClient has already been bumped to this newer version as part of a
separate task https://issues.apache.org/jira/browse/KAFKA-2169 and it's
already in trunk. This change in my review request only passes along an
(optional) value to the ZkClient constructor that was introduced in that
newer version.
>
>
>> https://reviews.apache.org/r/30403/ related to
>> https://issues.apache.org/jira/browse/KAFKA-1906
>
> I don't understand the patch and how it would fix the issue. I also don't
> think necessarily there is an issue. Its a balance from the community
> having a good out of the box experience vs taking defaults and rushing them
> into production. No matter what we do we can't stop the latter from
> happening, which will also cause issues.
The change to use a default directory that's within the Kafka
installation path rather than /tmp folder (which get erased on restarts)
is more from a development environment point of view rather than
production. As you note, production environments will anyway have to
deal with setting the right configs. From a developer perspective, I
like the Kafka logs to survive system restarts when I'm working on
applications which use Kafka. Of course, I can go ahead and change that
default value in the server.properties on each fresh installation. But
personally, I like it more if the logs are are stored within the Kafka
installation itself so that even if I have multiple different versions
of Kafka running (for different applications) on the same system, the
logs are isolated to the Kafka installation and don't interfere with
each other. We currently have a development setup where we have a bunch
of VMs with different Kafka installations. These VMs are then handed out
to developers to work on various different applications (which are under
development). The first thing we currently do is edit the
server.properties and update the log path (and that's the only change we
do for dev). It would be much more easier and convenient/manageable if
this log directory default to a path within the Kafka installation.
>
>>
>> There's also this one https://reviews.apache.org/r/34697/ for
>> https://issues.apache.org/jira/browse/KAFKA-2221 but it's only been up
>> since a couple of days and is a fairly minor one.
>
> Folks should start to transition in 0.8.3 to the new java consumer (which
> is on trunk). If this fix is so critical we should release it in 0.8.2.2
> otherwise continue to try to not make changes to the existing scalal
> consumer.
>
Fair enough. It was more to help narrow down the real issues when a
reconnect happens and isn't that critical. Do you want me to close that
review request?
-Jaikiran
Re: Nagging - pending review requests :)
Posted by Joe Stein <jo...@stealth.ly>.
see below
On Fri, May 29, 2015 at 2:25 AM, Jaikiran Pai <ja...@gmail.com>
wrote:
> Could someone please look at these few review requests and let me know if
> any changes are needed:
>
> https://reviews.apache.org/r/34394/ related to
> https://issues.apache.org/jira/browse/KAFKA-1907
I haven't looked at all the other changes that would be introduced from
their release that could break between zk and kafka by introducing a zk
client bump. A less ops negative way to deal with this might be to create a
plugable interface, then someone can use a patched zkclient if they wanted,
or exhibitor, or consul, or akka, etc.
>
> https://reviews.apache.org/r/30403/ related to
> https://issues.apache.org/jira/browse/KAFKA-1906
I don't understand the patch and how it would fix the issue. I also don't
think necessarily there is an issue. Its a balance from the community
having a good out of the box experience vs taking defaults and rushing them
into production. No matter what we do we can't stop the latter from
happening, which will also cause issues.
>
>
> There's also this one https://reviews.apache.org/r/34697/ for
> https://issues.apache.org/jira/browse/KAFKA-2221 but it's only been up
> since a couple of days and is a fairly minor one.
Folks should start to transition in 0.8.3 to the new java consumer (which
is on trunk). If this fix is so critical we should release it in 0.8.2.2
otherwise continue to try to not make changes to the existing scalal
consumer.
>
>
> -Jaikiran
>