Maybe I'm in the minority here, but I actually don't think there should be a default for this param and you should be required to explicitly set this.
On Thu, Jan 29, 2015 at 5:43 AM, Jay Kreps <boredandr...@gmail.com> wrote: > > > > 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? > > 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? > > > - Jay > > > ----------------------------------------------------------- > 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 > > > > > > -- Jeff Holoman Systems Engineer 678-612-9519