Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

set kafka offset to retention - segmentSize #850

Merged
merged 3 commits into from
Feb 22, 2018
Merged

set kafka offset to retention - segmentSize #850

merged 3 commits into from
Feb 22, 2018

Conversation

woodsaj
Copy link
Member

@woodsaj woodsaj commented Feb 13, 2018

If the kafka offset is set to "oldest", query zookeeper for the topic retention
and segment size. Then set the offset config var to retention - segmentSize.
This ensures that MT doesnt start consuming from the oldest log segment which
could be deleted while MT is still consuming it, which leads to MT having to shutdown and restart

this is a step towards improving backlog replay
relates to https://github.com/raintank/hosted-metrics-api/issues/82
and
issue #614

zk = KazooClient(hosts='zookeeper:2181')
zk.start()
try:
resp = zk.get("/config/topics/%s" % topic)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's safe to assume that kafka won't change that too often

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this wont change. Kafka 0.11 and newer have an AdminApi for querying/setting topic config, but there are no clients for it yet.

print "oldest"
sys.exit(0)

data = json.loads(resp[0])
Copy link
Contributor

@replay replay Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could throw a ValueError exception if there is malformed data, probably better to catch it and print oldest


data = json.loads(resp[0])

retention=int(data['config'].get('retention.ms', 0))
Copy link
Contributor

@replay replay Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could throw KeyError if config isn't present, data.get('config', {}).get('retention.ms', 0) should be safer. also ValueError if the value isn't numeric

sys.exit(0)

# oldest we can safetly consume from in minutes
oldest = int((retention - segmentSize)/(1000 * 60))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it necessary to convert that into int()? i think it should be int anyway

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i want an int not a float

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think python would make this a float

>>> segmentSize=int(100)
>>> retention=int(1234512345)
>>> (retention - segmentSize)/(1000 * 60)
20575
>>> type((retention - segmentSize)/(1000 * 60))
<type 'int'>

@replay
Copy link
Contributor

replay commented Feb 14, 2018

That's a nice idea. The only problem I see is if for some reason it stops working, like for example if kafka changed the location or format of that config, and if all the exceptions get handled and it just outputs oldest, then we'd probably never even realize that it broke. At the same time we'd probably want to handle all the possible exceptions to make sure that some corruption in zk can't prevent all MTs of an instance from starting

If the kafka offset is set to "oldest", query zookeeper for the topic retention
and segment size.  Then set the offset config var to retention - segmentSize.
This ensures that MT doesnt start consuming from the oldest log segment which
could be deleted while MT is still consuming it.
fi

if [ x"$MT_KAFKA_CLUSTER_OFFSET" = "xoldest" ]; then
MT_KAFKA_CLUSTER_OFFSET=$(/getOffset.py $MT_KAFKA_CLUSTER_TOPIC)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to export these vars

@Dieterbe
Copy link
Contributor

can we name this new option "smart" or "auto" or something, to avoid confusion and clearly distinguish is from "oldest".

Copy link
Contributor

@Dieterbe Dieterbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments

@Dieterbe Dieterbe merged commit 8d17f34 into master Feb 22, 2018
@Dieterbe Dieterbe deleted the betterOffset branch September 18, 2018 08:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants