Perhaps we could start with max.uncleanable.partitions and then implement max.uncleanable.partitions.per.logdir in a follow-up change if it seemed to be necessary? What do you think?
regards, Colin On Sat, Aug 4, 2018, at 10:53, Stanislav Kozlovski wrote: > Hey Ray, > > Thanks for the explanation. In regards to the configuration property - I'm > not sure. As long as it has sufficient documentation, I find > "max.uncleanable.partitions" to be okay. If we were to add the distinction > explicitly, maybe it should be `max.uncleanable.partitions.per.logdir` ? > > On Thu, Aug 2, 2018 at 7:32 PM Ray Chiang <rchi...@apache.org> wrote: > > > One more thing occurred to me. Should the configuration property be > > named "max.uncleanable.partitions.per.disk" instead? > > > > -Ray > > > > > > On 8/1/18 9:11 AM, Stanislav Kozlovski wrote: > > > Yes, good catch. Thank you, James! > > > > > > Best, > > > Stanislav > > > > > > On Wed, Aug 1, 2018 at 5:05 PM James Cheng <wushuja...@gmail.com> wrote: > > > > > >> Can you update the KIP to say what the default is for > > >> max.uncleanable.partitions? > > >> > > >> -James > > >> > > >> Sent from my iPhone > > >> > > >>> On Jul 31, 2018, at 9:56 AM, Stanislav Kozlovski < > > stanis...@confluent.io> > > >> wrote: > > >>> Hey group, > > >>> > > >>> I am planning on starting a voting thread tomorrow. Please do reply if > > >> you > > >>> feel there is anything left to discuss. > > >>> > > >>> Best, > > >>> Stanislav > > >>> > > >>> On Fri, Jul 27, 2018 at 11:05 PM Stanislav Kozlovski < > > >> stanis...@confluent.io> > > >>> wrote: > > >>> > > >>>> Hey, Ray > > >>>> > > >>>> Thanks for pointing that out, it's fixed now > > >>>> > > >>>> Best, > > >>>> Stanislav > > >>>> > > >>>>> On Fri, Jul 27, 2018 at 9:43 PM Ray Chiang <rchi...@apache.org> > > wrote: > > >>>>> > > >>>>> Thanks. Can you fix the link in the "KIPs under discussion" table on > > >>>>> the main KIP landing page > > >>>>> < > > >>>>> > > >> > > https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals# > > >>> ? > > >>>>> I tried, but the Wiki won't let me. > > >>>>> > > >>>>> -Ray > > >>>>> > > >>>>>> On 7/26/18 2:01 PM, Stanislav Kozlovski wrote: > > >>>>>> Hey guys, > > >>>>>> > > >>>>>> @Colin - good point. I added some sentences mentioning recent > > >>>>> improvements > > >>>>>> in the introductory section. > > >>>>>> > > >>>>>> *Disk Failure* - I tend to agree with what Colin said - once a disk > > >>>>> fails, > > >>>>>> you don't want to work with it again. As such, I've changed my mind > > >> and > > >>>>>> believe that we should mark the LogDir (assume its a disk) as > > offline > > >> on > > >>>>>> the first `IOException` encountered. This is the LogCleaner's > > current > > >>>>>> behavior. We shouldn't change that. > > >>>>>> > > >>>>>> *Respawning Threads* - I believe we should never re-spawn a thread. > > >> The > > >>>>>> correct approach in my mind is to either have it stay dead or never > > >> let > > >>>>> it > > >>>>>> die in the first place. > > >>>>>> > > >>>>>> *Uncleanable-partition-names metric* - Colin is right, this metric > > is > > >>>>>> unneeded. Users can monitor the `uncleanable-partitions-count` > > metric > > >>>>> and > > >>>>>> inspect logs. > > >>>>>> > > >>>>>> > > >>>>>> Hey Ray, > > >>>>>> > > >>>>>>> 2) I'm 100% with James in agreement with setting up the LogCleaner > > to > > >>>>>>> skip over problematic partitions instead of dying. > > >>>>>> I think we can do this for every exception that isn't `IOException`. > > >>>>> This > > >>>>>> will future-proof us against bugs in the system and potential other > > >>>>> errors. > > >>>>>> Protecting yourself against unexpected failures is always a good > > thing > > >>>>> in > > >>>>>> my mind, but I also think that protecting yourself against bugs in > > the > > >>>>>> software is sort of clunky. What does everybody think about this? > > >>>>>> > > >>>>>>> 4) The only improvement I can think of is that if such an > > >>>>>>> error occurs, then have the option (configuration setting?) to > > >> create a > > >>>>>>> <log_segment>.skip file (or something similar). > > >>>>>> This is a good suggestion. Have others also seen corruption be > > >> generally > > >>>>>> tied to the same segment? > > >>>>>> > > >>>>>> On Wed, Jul 25, 2018 at 11:55 AM Dhruvil Shah <dhru...@confluent.io > > > > > >>>>> wrote: > > >>>>>>> For the cleaner thread specifically, I do not think respawning will > > >>>>> help at > > >>>>>>> all because we are more than likely to run into the same issue > > again > > >>>>> which > > >>>>>>> would end up crashing the cleaner. Retrying makes sense for > > transient > > >>>>>>> errors or when you believe some part of the system could have > > healed > > >>>>>>> itself, both of which I think are not true for the log cleaner. > > >>>>>>> > > >>>>>>> On Wed, Jul 25, 2018 at 11:08 AM Ron Dagostino <rndg...@gmail.com> > > >>>>> wrote: > > >>>>>>>> <<<respawning threads is likely to make things worse, by putting > > you > > >>>>> in > > >>>>>>> an > > >>>>>>>> infinite loop which consumes resources and fires off continuous > > log > > >>>>>>>> messages. > > >>>>>>>> Hi Colin. In case it could be relevant, one way to mitigate this > > >>>>> effect > > >>>>>>> is > > >>>>>>>> to implement a backoff mechanism (if a second respawn is to occur > > >> then > > >>>>>>> wait > > >>>>>>>> for 1 minute before doing it; then if a third respawn is to occur > > >> wait > > >>>>>>> for > > >>>>>>>> 2 minutes before doing it; then 4 minutes, 8 minutes, etc. up to > > >> some > > >>>>> max > > >>>>>>>> wait time). > > >>>>>>>> > > >>>>>>>> I have no opinion on whether respawn is appropriate or not in this > > >>>>>>> context, > > >>>>>>>> but a mitigation like the increasing backoff described above may > > be > > >>>>>>>> relevant in weighing the pros and cons. > > >>>>>>>> > > >>>>>>>> Ron > > >>>>>>>> > > >>>>>>>> On Wed, Jul 25, 2018 at 1:26 PM Colin McCabe <cmcc...@apache.org> > > >>>>> wrote: > > >>>>>>>>>> On Mon, Jul 23, 2018, at 23:20, James Cheng wrote: > > >>>>>>>>>> Hi Stanislav! Thanks for this KIP! > > >>>>>>>>>> > > >>>>>>>>>> I agree that it would be good if the LogCleaner were more > > tolerant > > >>>>> of > > >>>>>>>>>> errors. Currently, as you said, once it dies, it stays dead. > > >>>>>>>>>> > > >>>>>>>>>> Things are better now than they used to be. We have the metric > > >>>>>>>>>> > > kafka.log:type=LogCleanerManager,name=time-since-last-run-ms > > >>>>>>>>>> which we can use to tell us if the threads are dead. And as of > > >>>>> 1.1.0, > > >>>>>>>> we > > >>>>>>>>>> have KIP-226, which allows you to restart the log cleaner > > thread, > > >>>>>>>>>> without requiring a broker restart. > > >>>>>>>>>> > > >> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-226+-+Dynamic+Broker+Configuration > > >>>>>>>>>> < > > >> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-226+-+Dynamic+Broker+Configuration > > >>>>>>>>>> I've only read about this, I haven't personally tried it. > > >>>>>>>>> Thanks for pointing this out, James! Stanislav, we should > > probably > > >>>>>>> add a > > >>>>>>>>> sentence or two mentioning the KIP-226 changes somewhere in the > > >> KIP. > > >>>>>>>> Maybe > > >>>>>>>>> in the intro section? > > >>>>>>>>> > > >>>>>>>>> I think it's clear that requiring the users to manually restart > > the > > >>>>> log > > >>>>>>>>> cleaner is not a very good solution. But it's good to know that > > >>>>> it's a > > >>>>>>>>> possibility on some older releases. > > >>>>>>>>> > > >>>>>>>>>> Some comments: > > >>>>>>>>>> * I like the idea of having the log cleaner continue to clean as > > >>>>> many > > >>>>>>>>>> partitions as it can, skipping over the problematic ones if > > >>>>> possible. > > >>>>>>>>>> * If the log cleaner thread dies, I think it should > > automatically > > >> be > > >>>>>>>>>> revived. Your KIP attempts to do that by catching exceptions > > >> during > > >>>>>>>>>> execution, but I think we should go all the way and make sure > > >> that a > > >>>>>>>> new > > >>>>>>>>>> one gets created, if the thread ever dies. > > >>>>>>>>> This is inconsistent with the way the rest of Kafka works. We > > >> don't > > >>>>>>>>> automatically re-create other threads in the broker if they > > >>>>> terminate. > > >>>>>>>> In > > >>>>>>>>> general, if there is a serious bug in the code, respawning > > threads > > >> is > > >>>>>>>>> likely to make things worse, by putting you in an infinite loop > > >> which > > >>>>>>>>> consumes resources and fires off continuous log messages. > > >>>>>>>>> > > >>>>>>>>>> * It might be worth trying to re-clean the uncleanable > > partitions. > > >>>>>>> I've > > >>>>>>>>>> seen cases where an uncleanable partition later became > > cleanable. > > >> I > > >>>>>>>>>> unfortunately don't remember how that happened, but I remember > > >> being > > >>>>>>>>>> surprised when I discovered it. It might have been something > > like > > >> a > > >>>>>>>>>> follower was uncleanable but after a leader election happened, > > the > > >>>>>>> log > > >>>>>>>>>> truncated and it was then cleanable again. I'm not sure. > > >>>>>>>>> James, I disagree. We had this behavior in the Hadoop > > Distributed > > >>>>> File > > >>>>>>>>> System (HDFS) and it was a constant source of user problems. > > >>>>>>>>> > > >>>>>>>>> What would happen is disks would just go bad over time. The > > >> DataNode > > >>>>>>>>> would notice this and take them offline. But then, due to some > > >>>>>>>>> "optimistic" code, the DataNode would periodically try to re-add > > >> them > > >>>>>>> to > > >>>>>>>>> the system. Then one of two things would happen: the disk would > > >> just > > >>>>>>>> fail > > >>>>>>>>> immediately again, or it would appear to work and then fail > > after a > > >>>>>>> short > > >>>>>>>>> amount of time. > > >>>>>>>>> > > >>>>>>>>> The way the disk failed was normally having an I/O request take a > > >>>>>>> really > > >>>>>>>>> long time and time out. So a bunch of request handler threads > > >> would > > >>>>>>>>> basically slam into a brick wall when they tried to access the > > bad > > >>>>>>> disk, > > >>>>>>>>> slowing the DataNode to a crawl. It was even worse in the second > > >>>>>>>> scenario, > > >>>>>>>>> if the disk appeared to work for a while, but then failed. Any > > >> data > > >>>>>>> that > > >>>>>>>>> had been written on that DataNode to that disk would be lost, and > > >> we > > >>>>>>>> would > > >>>>>>>>> need to re-replicate it. > > >>>>>>>>> > > >>>>>>>>> Disks aren't biological systems-- they don't heal over time. > > Once > > >>>>>>>> they're > > >>>>>>>>> bad, they stay bad. The log cleaner needs to be robust against > > >> cases > > >>>>>>>> where > > >>>>>>>>> the disk really is failing, and really is returning bad data or > > >>>>> timing > > >>>>>>>> out. > > >>>>>>>>>> * For your metrics, can you spell out the full metric in > > JMX-style > > >>>>>>>>>> format, such as: > > >>>>>>>>>> > > >>>>>>>> > > kafka.log:type=LogCleanerManager,name=uncleanable-partitions-count > > >>>>>>>>>> value=4 > > >>>>>>>>>> > > >>>>>>>>>> * For "uncleanable-partitions": topic-partition names can be > > very > > >>>>>>> long. > > >>>>>>>>>> I think the current max size is 210 characters (or maybe > > >> 240-ish?). > > >>>>>>>>>> Having the "uncleanable-partitions" being a list could be very > > >> large > > >>>>>>>>>> metric. Also, having the metric come out as a csv might be > > >> difficult > > >>>>>>> to > > >>>>>>>>>> work with for monitoring systems. If we *did* want the topic > > names > > >>>>> to > > >>>>>>>> be > > >>>>>>>>>> accessible, what do you think of having the > > >>>>>>>>>> kafka.log:type=LogCleanerManager,topic=topic1,partition=2 > > >>>>>>>>>> I'm not sure if LogCleanerManager is the right type, but my > > >> example > > >>>>>>> was > > >>>>>>>>>> that the topic and partition can be tags in the metric. That > > will > > >>>>>>> allow > > >>>>>>>>>> monitoring systems to more easily slice and dice the metric. I'm > > >> not > > >>>>>>>>>> sure what the attribute for that metric would be. Maybe > > something > > >>>>>>> like > > >>>>>>>>>> "uncleaned bytes" for that topic-partition? Or > > >>>>> time-since-last-clean? > > >>>>>>>> Or > > >>>>>>>>>> maybe even just "Value=1". > > >>>>>>>>> I haven't though about this that hard, but do we really need the > > >>>>>>>>> uncleanable topic names to be accessible through a metric? It > > >> seems > > >>>>>>> like > > >>>>>>>>> the admin should notice that uncleanable partitions are present, > > >> and > > >>>>>>> then > > >>>>>>>>> check the logs? > > >>>>>>>>> > > >>>>>>>>>> * About `max.uncleanable.partitions`, you said that this likely > > >>>>>>>>>> indicates that the disk is having problems. I'm not sure that is > > >> the > > >>>>>>>>>> case. For the 4 JIRAs that you mentioned about log cleaner > > >> problems, > > >>>>>>>> all > > >>>>>>>>>> of them are partition-level scenarios that happened during > > normal > > >>>>>>>>>> operation. None of them were indicative of disk problems. > > >>>>>>>>> I don't think this is a meaningful comparison. In general, we > > >> don't > > >>>>>>>>> accept JIRAs for hard disk problems that happen on a particular > > >>>>>>> cluster. > > >>>>>>>>> If someone opened a JIRA that said "my hard disk is having > > >> problems" > > >>>>> we > > >>>>>>>>> could close that as "not a Kafka bug." This doesn't prove that > > >> disk > > >>>>>>>>> problems don't happen, but just that JIRA isn't the right place > > >> for > > >>>>>>>> them. > > >>>>>>>>> I do agree that the log cleaner has had a significant number of > > >> logic > > >>>>>>>>> bugs, and that we need to be careful to limit their impact. > > That's > > >>>>> one > > >>>>>>>>> reason why I think that a threshold of "number of uncleanable > > logs" > > >>>>> is > > >>>>>>> a > > >>>>>>>>> good idea, rather than just failing after one IOException. In > > all > > >>>>> the > > >>>>>>>>> cases I've seen where a user hit a logic bug in the log cleaner, > > it > > >>>>> was > > >>>>>>>>> just one partition that had the issue. We also should increase > > >> test > > >>>>>>>>> coverage for the log cleaner. > > >>>>>>>>> > > >>>>>>>>>> * About marking disks as offline when exceeding a certain > > >> threshold, > > >>>>>>>>>> that actually increases the blast radius of log compaction > > >> failures. > > >>>>>>>>>> Currently, the uncleaned partitions are still readable and > > >> writable. > > >>>>>>>>>> Taking the disks offline would impact availability of the > > >>>>> uncleanable > > >>>>>>>>>> partitions, as well as impact all other partitions that are on > > the > > >>>>>>>> disk. > > >>>>>>>>> In general, when we encounter I/O errors, we take the disk > > >> partition > > >>>>>>>>> offline. This is spelled out in KIP-112 ( > > >>>>>>>>> > > >> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-112%3A+Handle+disk+failure+for+JBOD > > >>>>>>>>> ) : > > >>>>>>>>> > > >>>>>>>>>> - Broker assumes a log directory to be good after it starts, and > > >>>>> mark > > >>>>>>>>> log directory as > > >>>>>>>>>> bad once there is IOException when broker attempts to access > > (i.e. > > >>>>>>> read > > >>>>>>>>> or write) the log directory. > > >>>>>>>>>> - Broker will be offline if all log directories are bad. > > >>>>>>>>>> - Broker will stop serving replicas in any bad log directory. > > New > > >>>>>>>>> replicas will only be created > > >>>>>>>>>> on good log directory. > > >>>>>>>>> The behavior Stanislav is proposing for the log cleaner is > > actually > > >>>>>>> more > > >>>>>>>>> optimistic than what we do for regular broker I/O, since we will > > >>>>>>> tolerate > > >>>>>>>>> multiple IOExceptions, not just one. But it's generally > > >> consistent. > > >>>>>>>>> Ignoring errors is not. In any case, if you want to tolerate an > > >>>>>>>> unlimited > > >>>>>>>>> number of I/O errors, you can just set the threshold to an > > infinite > > >>>>>>> value > > >>>>>>>>> (although I think that would be a bad idea). > > >>>>>>>>> > > >>>>>>>>> best, > > >>>>>>>>> Colin > > >>>>>>>>> > > >>>>>>>>>> -James > > >>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>>> On Jul 23, 2018, at 5:46 PM, Stanislav Kozlovski < > > >>>>>>>>> stanis...@confluent.io> wrote: > > >>>>>>>>>>> I renamed the KIP and that changed the link. Sorry about that. > > >> Here > > >>>>>>>> is > > >>>>>>>>> the > > >>>>>>>>>>> new link: > > >>>>>>>>>>> > > >> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-346+-+Improve+LogCleaner+behavior+on+error > > >>>>>>>>>>> On Mon, Jul 23, 2018 at 5:11 PM Stanislav Kozlovski < > > >>>>>>>>> stanis...@confluent.io> > > >>>>>>>>>>> wrote: > > >>>>>>>>>>> > > >>>>>>>>>>>> Hey group, > > >>>>>>>>>>>> > > >>>>>>>>>>>> I created a new KIP about making log compaction more > > >>>>>>> fault-tolerant. > > >>>>>>>>>>>> Please give it a look here and please share what you think, > > >>>>>>>>> especially in > > >>>>>>>>>>>> regards to the points in the "Needs Discussion" paragraph. > > >>>>>>>>>>>> > > >>>>>>>>>>>> KIP: KIP-346 > > >>>>>>>>>>>> < > > >> > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-346+-+Limit+blast+radius+of+log+compaction+failure > > >>>>>>>>>>>> -- > > >>>>>>>>>>>> Best, > > >>>>>>>>>>>> Stanislav > > >>>>>>>>>>>> > > >>>>>>>>>>> -- > > >>>>>>>>>>> Best, > > >>>>>>>>>>> Stanislav > > >>>>> > > >>>> -- > > >>>> Best, > > >>>> Stanislav > > >>>> > > >>> > > >>> -- > > >>> Best, > > >>> Stanislav > > > > > > > > > -- > Best, > Stanislav
