> On Jan. 29, 2015, 6:50 a.m., Gwen Shapira wrote:
> > We added --override option to KafkaServer that allows overriding default
> > configuration from commandline.
> > I believe that just changing the shell script to include --override
> > log.dir=${KAFKA_HOME}/data
> > may be enough?
> >
> > overriding configuration from server.properties in code can be very
> > unintuitive.
>
> Jaikiran Pai wrote:
> That sounds a good idea. I wasn't aware of the --override option. I'll
> give that a try and if it works then the change will be limited to just the
> scripts.
>
> Jaikiran Pai wrote:
> Hi Gwen,
>
> I had a look at the JIRA https://issues.apache.org/jira/browse/KAFKA-1654
> which added support for --override and also the purpose of that option. From
> what I see it won't be useful in this case, because in this current task, we
> don't want to override a value that has been explicitly set (via
> server.properties for example). Instead we want to handle a case where no
> explicit value is specified for the data log directory and in such cases
> default it to a path which resides under the Kafka install directory.
>
> If we use the --override option in our (startup) scripts to set
> log.dir=${KAFKA_HOME}/data, we will end up forcing this value as the log.dir
> even when the user has intentionally specified a different path for the data
> logs.
>
> Let me know if I misunderstood your suggestion.
>
> Jay Kreps wrote:
> I think you are right that --override won't work but maybe this is still
> a good suggestion?
>
> Something seems odd about force overriding the working directory of the
> process just to set the log directory.
>
> Another option might be to add --default. This would work like --override
> but would provide a default value only if none is specified. I think this
> might be possible because the java.util.Properties we use for config supports
> a hierarchy of defaults. E.g. you can say new Properties(defaultProperties).
> Not sure if this is better or worse.
>
> Thoughts?
>
> Jaikiran Pai wrote:
> Hi Jay,
>
> > Another option might be to add --default. This would work like
> --override but would provide a default value only if none is specified. I
> think this might be possible because the java.util.Properties we use for
> config supports a hierarchy of defaults. E.g. you can say new
> Properties(defaultProperties). Not sure if this is better or worse.
>
> I think --default sounds like a good idea which could help us use it for
> other properties too (if we need to). It does look better than the current
> change that I have done, because the Java code then doesn't have to worry
> about how that default value is sourced. We can then just update the scripts
> to set up the default for the log.dir appropriately.
>
> I can work towards adding support for it and will update this patch once
> it's ready.
>
> As for:
>
> > Something seems odd about force overriding the working directory of the
> process just to set the log directory.
>
> Sorry, I don't understand what you meant there. Is it something about the
> change that was done to the scripts?
>
> Jay Kreps wrote:
> I guess what I mean is: is there any other reason you might care about
> the working directory of the process? If so we probably don't want to force
> it to be the Kafka directory. If not it may actually be fine and in that case
> I think having relative paths work is nice. I don't personally know the
> answer to this, what is "good practice" for a server process?
Looking at my favorite projects:
Zookeeper defaults to /tmp/zookeeper
HDFS defaults to an empty configuration file and will not start without an
explicit disk location.
MySQL defaults to a data/ or var/ directory under the base_dir (depending on
whether its a binary package or compiled from source...)
Lots of standards, I think we are free to choose one :)
- Gwen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30403/#review70168
-----------------------------------------------------------
On Jan. 29, 2015, 6:24 a.m., Jaikiran Pai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30403/
> -----------------------------------------------------------
>
> (Updated Jan. 29, 2015, 6:24 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-1906
> https://issues.apache.org/jira/browse/KAFKA-1906
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-1906 Default the Kafka data log directory to
> $KAFKA_HOME/data/kafka-logs directory, where KAFKA_HOME is the Kafka
> installation directory
>
>
> Diffs
> -----
>
> bin/kafka-run-class.sh 881f578a8f5c796fe23415b978c1ad35869af76e
> bin/windows/kafka-run-class.bat 9df3d2b45236b4f06d55a89c84afcf0ab9f5d0f2
> config/server.properties 1614260b71a658b405bb24157c8f12b1f1031aa5
> core/src/main/scala/kafka/server/KafkaConfig.scala
> 6d74983472249eac808d361344c58cc2858ec971
> core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala
> 82dce80d553957d8b5776a9e140c346d4e07f766
>
> Diff: https://reviews.apache.org/r/30403/diff/
>
>
> Testing
> -------
>
> The change here involves updating the Kafka scripts (for Windows and * nix)
> to infer and setup KAFKA_HOME environment variable. This value is then used
> by the KafkaConfig to decide what path to default to for the Kafka data logs,
> in the absence of any explicitly set log.dirs (or log.dir) properties.
>
> Care has been taken to ensure that other mechanism which might presently be
> bypassing the Kafka scripts, will still continue to function, since in the
> absence of KAFKA_HOME environment property value, we fall back to
> /tmp/kafka-logs (the present default) as the default data log directory
>
> Existing tests have been run to ensure that this change maintains backward
> compatibility (i.e. doesn't fail when KAFKA_HOME isn't available/set) and 2
> new test methods have been added to the KafkaConfigTest to ensure that this
> change works.
>
> Although the change has been made to both .sh and .bat files, to support
> this, I haven't actually tested this change on a Windows OS and would
> appreciate if someone can test this there and let me know if they run into
> any issues.
>
>
> Thanks,
>
> Jaikiran Pai
>
>
