Homepage performance improvement

[jorkzijlstra]

This refactoring makes it possible to load in my case 1600 topic with 11000 topic partition in 30 seconds (tested with a kafka stack that is other side of the world which means a higher ttl) and consists of several changes. Without these changes it was impossible to load the homepage.

Main point is that this can be achieved by not going to kafka where its not needed, or combine several calls into one.

Total kafka calls removed:

• (n * topic kafkaConsumer.beginningOffsets) - 1
• (n * topic kafkaConsumer.endOffsets) - 1
• n * topic kafkaConsumer.partitionsFor

TopicInfo

When retrieving the topicInfo it was first doing the listTopics and subsequenty looping over them to retrieve the partitions. The partition data is however already part of the listTopics data. Instead of going to kafka again for the data for each topic I'm just getting it from the listTopics data.

Old version:

• 1 listTopics call
• n * topic kafkaConsumer.partitionsFor call

New version

• 1 listTopics call

PartitionSizes

When getting the partition sizes the code now get the partition start en end offset by doing 2 call per topic. For the kafka consumer call you only give it the partitions of the topic, not the topic. Instead of giving it the partition of 1 topic why not all partition at once.

Old version:

• n * topic kafkaConsumer.beginningOffsets
• n * topic kafkaConsumer.endOffsets

New version

• 1 * kafkaConsumer.beginningOffsets
• 1 * kafkaConsumer.endOffsets

Homepage improvement

On the homepage and other pages we actually don't need the partition sizes but its being retrieved regardless. So I splitted the getTopics in 2 methods, one with and one without the partition sizes

Old version:

• n * topic kafkaConsumer.beginningOffsets

New version

• omitted

Fix #246
Fix #233

[jorkzijlstra]

I just now saw this one: https://github.com/obsidiandynamics/kafdrop/pull/142/files

Seeing the comment there I probably need to be splitting up that TopicVO as well and see if I can keep the partitionInfoList out of the TopicVO since its only used internally to pass along the values, not for outputting

[jorkzijlstra]

It would be nice if someone can review and give some comments so that I can hopefully finalize this and get it merged.

[davideicardi]

For what I can understand it seems to be a very good improvement. Thank you!

I just have some concerns on the code style. For example can we rewrite function

synchronized void setAllPartitionSizes(Map<String, List<PartitionInfo>> topicsMap, List<TopicVO> topics)

to something like

synchronized Map<String, List<PartitionInfo>> getPartitionsSizes(List<TopicVO> topics)

I suspect that you have done some of the changes for performance reasons, but these kind of functions can be difficult to test and can have some strange side effects.

What do you think?

[jorkzijlstra]

@davideicardi

All these changes are basically just reducing the number of call to fetch kafka data or fetch less data.

It has been a while since I wrote this and I actually don't remember why I made it a side effecting function. Normally I never would have done that so I suspect I had a good reason at the time. That said I totally agree that is shouldn't have to be.

The offsets are only part the TopicPartitionVO which is part of the TopicVO and not of the PartitionInfo.
Aah so that is why I made it a side effecting functions. The TopicVO is the one that we inject and which needs updating.
If I where to implement it now I would have written something like synchronized List<TopicVO> topics withPartitionsSizes(List<TopicVO> topics) where I would duplicate the topicVO and return the new one with the partitionSizes set.

The other option would I think be synchronized Map<String, Map<Integer, TopicPartitionVO>> getPartitionsSizes(List<TopicVO> topics) eg Map<topicname, Map<partitionId, TopicPartitionVO>>. This way we have all the identifiers (topicName and paritionId) which we need to set this data.

I'm curious what you think about which direction I should go.

[davideicardi]

@jorkzijlstra

Consider that I'm not an expert on this project, I'm just trying to maintain it ;-), but from what I understand I prefer your first solution.
I just suggest to call the function calculatePartitionsSizes or something similar.

synchronized List<TopicVO> calculatePartitionsSizes(List<TopicVO> topics)

Also here some kind of unit test can be useful, but I agree that can be difficult ... consider it as a "nice to have"!

Thank you very much for you help and explanation!

[jorkzijlstra]

@davideicardi Thanks for starting to maintain it and the same as you I'm also not an expert on the project.

I'm also not a Java programmer and don't know the programming standards for this particular project, which is why I was asking input which route I should take.

I only did some cleanup yet but haven't had the time to change the method signature yet.

[jorkzijlstra]

@davideicardi I have tried to go the route of returning a new TopicVO with the updated data. I did not finish writing the code because I felt it became way to ugly and it also requires a deepcopy of the TopicVO and all it partitionData.

I thought that copying the data and returning an updated version would be trivial to do. At least this is what I'm used to in Scala. However making a deepcopy in Java doesn't seem to be trivial at all. I would need to add a lot more code.

On the /consumer/:id route we are also fetching all topics, including offset and filtering down on topic where that group actually has any offsets available. By any means we are discarding almost all the data we are fetching from kafka,

Taking this into account I feel that, with the current data model, I unfortunately don't see another option than to make it a side effecting function. So I left the method name as is, since it actually setting the partitionsizes.

What are your thoughts about this?

[jorkzijlstra]

@davideicardi

I mentioned the consumer group issue previously and worked on that a bit and removed the getTopicsWithOffsets method again in this PR: jorkzijlstra#1

[davideicardi]

LGTM!

Let's wait a couple of days to see if someone has any comment and then for me we can merge it.

[jorkzijlstra]

I also just now merged jorkzijlstra#1 since its also cleaning up a method.
As for testing I did see there are some placeholder tests created, but haven't yet looked into creating some.

[jorkzijlstra]

I haven't had the chance to have a look into adding unit / integrations test. Despite this are you considering that the couple of days have passed already?

[jorkzijlstra]

@davideicardi Many thanks for starting to maintain this repo.