Hey Jai, see below On Fri, May 29, 2015 at 3:03 AM, Jaikiran Pai <jai.forums2...@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 <jai.forums2...@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 >
