> On Aug. 14, 2014, 9:52 p.m., Neha Narkhede wrote: > > core/src/main/scala/kafka/server/KafkaConfig.scala, line 323 > > <https://reviews.apache.org/r/24704/diff/1/?file=660645#file660645line323> > > > > Can we just call the config compression.type? Also, the default value > > of NoCompressionCodec is a little misleading. It gives the impression that > > if compressed data is sent, the broker would write it as uncompressed. > > Maybe we could specify that explicitly in the config doc? > > Manikumar Reddy O wrote: > ok. I renamed the "default.compression.type" to "compression.type". This > config property is applicable to all non-compressed messages received on the > broker. Yes, we should explicitly specify this in documentation. > > Joel Koshy wrote: > Manikumar/Neha - would it be less confusing if we add another config as > well. Something like "broker.compression.enabled". If that is true, then > always honor "broker.compression.type" even if it is NoCompressionCodec > > The current patch behavior may be a bit confusing: Suppose the default > compression.type is GZIP. Although I cannot think of a very compelling > use-case for this, if someone intentionally overrides a topic's > compression.type to NoCompressioNCodec that override will be ignored. > > Neha Narkhede wrote: > Something like "broker.compression.enabled". If that is true, then always > honor "broker.compression.type" even if it is NoCompressionCodec > > +1. That would certainly make it less confusing. > > Manikumar Reddy O wrote: > Joel - We will not ignore any per-topic overrides. per-topic override > always be used. > > If default compression.type is GZIP, if someone intentionally overrides a > topic's compression.type to NoCompressionCodec then NoCompressionCodec will > be used for that topic. > > Current patch supports the following behavior > > broker per-topic final-compression > compression.type compression.type for topic > > none (default) - none > gzip - gzip > gzip none none > gzip snappy snappy > > > And above is applicable to only non-compressed messages received on > broker. > > Do you still think we need a new "broker.compression.enabled" property? > > Joel Koshy wrote: > Sorry if I'm misreading (let me know). From what I can see, the current > condition (line 371) in the latest diff is: > > if (config.compressionType != NoCompressionCodec && codec == > NoCompressionCodec) > codec = config.compressionType > > So suppose a specific topic's override is NoCompressionCodec and an > incoming message-set has a gzip'd message in it. So codec would be set to > gzip (line 367). So the condition (line 371) fails and codec will not be > changed to NoCompressionCodec. > > > above is applicable to only non-compressed messages received on broker. > > This also leaves some room for confusion. Ideally, if there is an > explicit per-topic override, then that should always be used for that topic. > > So for example, if a topic's override is snappy; and an incoming message > is gzip. Then the final codec will be gzip - i..e, codec set to gzip (line > 367) and then the update is not applied because codec is not > NoCompressionCodec (line 371). When a user specifies an override for a config > named compression.type it is unintuitive if it is possible for the messages > to be of mixed compression types. This is not a contrived use case - say, if > an organization wants to immediately move its stored data from gzip to snappy > but cannot immediately move all its producers over from gzip to snappy. > > So does that mean the above condition should be removed altogether and we > should always set codec to config.compressionType? Not really, because > presumably the default compression.type will be NoCompressionCodec and that > is also not ideal - i.e., everything sent to the broker will be stored > uncompressed. That's perhaps where a broker.compression.enabled will be > useful.
Thanks for the detailed reply. I think, I misunderstood the requirement. My assumption was, we need not re-compress the compressed messages. I will do the following changes (1) I will add new "broker.compression.enabled" property (2) If broker.compression.enabled=true, then broker will re-compress the received messages to configured compression type, irrespective of their original compression. - Manikumar Reddy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24704/#review50651 ----------------------------------------------------------- On Aug. 15, 2014, 8:53 a.m., Manikumar Reddy O wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24704/ > ----------------------------------------------------------- > > (Updated Aug. 15, 2014, 8:53 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1499 > https://issues.apache.org/jira/browse/KAFKA-1499 > > > Repository: kafka > > > Description > ------- > > Addressing Neha's,Joel's comments > > > Diffs > ----- > > core/src/main/scala/kafka/log/Log.scala > 0ddf97bd30311b6039e19abade41d2fbbad2f59b > core/src/main/scala/kafka/log/LogConfig.scala > 5746ad4767589594f904aa085131dd95e56d72bb > core/src/main/scala/kafka/server/KafkaConfig.scala > 1a45f8716ccc0398cf9395d91d66199d16882aae > core/src/main/scala/kafka/server/KafkaServer.scala > 28711182aaa70eaa623de858bc063cb2613b2a4d > core/src/test/scala/unit/kafka/log/LogTest.scala > 577d102fc2eb6bb1a72326141ecd431db6d66f04 > core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala > 2377abe4933e065d037828a214c3a87e1773a8ef > > Diff: https://reviews.apache.org/r/24704/diff/ > > > Testing > ------- > > > Thanks, > > Manikumar Reddy O > >
