Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kgo: fix cancellation of a fetch in manageFetchConcurrency #921

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iimos
Copy link

@iimos iimos commented Mar 4, 2025

I discovered a "bug" in the code responsible for handling the cancellation of a fetch request. Fortunately, it doesn’t seem to cause any immediate issues (if I’ve correctly understood the relationship between manageFetchConcurrency and fetchLoop). However I think it’s better to fix it now to prevent potential problems in the future when changes are made.

In this snippet, instead of deleting the i-th element, the last element is being removed:

_ = append(wantFetch[i:], wantFetch[i+1:]...)
wantFetch = wantFetch[:len(wantFetch)-1]

I also attempted to write a test to cover this case, but it would require a significant redesign of manageFetchConcurrency so I’ve set this idea aside.

@twmb
Copy link
Owner

twmb commented Mar 7, 2025

This looks good, since you aren't seeing an active issue with the current code, I'm going to delay merging this until I ready the 1.19 release. With that, I'm currently stuck on what I believe is a Kafka bug -- I have two more things to try for evidence gathering before I create a KAFKA ticket.

@twmb
Copy link
Owner

twmb commented Mar 7, 2025

Actually, looking more closely, your form is a more succinct version of the existing code. The current code shifts the end of the slice down by one and trims off the end. Your code follows the more standard pattern of appending out the element you want to delete which automatically shifts the remaining slice down and trims off the end. The net new, besides cleaner & more idiomatic code, here is a break.

Overall, +1 thank you for the cleanup :)

@twmb twmb added the patch label Mar 7, 2025
@iimos
Copy link
Author

iimos commented Mar 9, 2025

your form is a more succinct version of the existing code

This is false. Here is the reproducer: https://go.dev/play/p/lMNhSm7abox

The current code shifts the end of the slice down by one and trims off the end.

Look carefully at:

_ = append(wantFetch[i:], wantFetch[i+1:]...)

Here is the tail is appended to the tail. To shift the tail down by one, two letters should be swapped: [i:] -> [:i]
This doesn't cause an issue only because it results in the recreation of the entire wantFetch slice, which is then discarded due to the _ =

@iimos
Copy link
Author

iimos commented Mar 9, 2025

The linter is complaining that .golangci.yml doesn't match json schema. Should i fix it in this PR or will it be handled without my involvement?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants