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

receive: port expanded postings cache from Cortex #7914

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

GiedriusS
Copy link
Member

@GiedriusS GiedriusS commented Nov 15, 2024

Port expanded postings cache from Cortex. Huge kudos to @alanprot for the implementation. I added a TODO item to convert our whole internal caching infra to be promise based.

@alanprot
Copy link
Contributor

Nice.

Maybe if we have this code here, i can make cortex use it and we don't have the code duplicated across both repos. :D

@GiedriusS GiedriusS force-pushed the expanded_postings_cache_receiver branch 4 times, most recently from 6312376 to 1383d92 Compare November 18, 2024 12:37
@GiedriusS
Copy link
Member Author

👍 good point. I see a panic in iter.Pull2 from time to time so I am keeping this a draft until I figure out what's wrong.

@alanprot
Copy link
Contributor

can you share the go routine trace when the panic happens?

@GiedriusS GiedriusS force-pushed the expanded_postings_cache_receiver branch 2 times, most recently from 932ea94 to 5d1ff7b Compare November 19, 2024 10:19
@GiedriusS
Copy link
Member Author

can you share the go routine trace when the panic happens?

I think I somehow ported some older version of the code which didn't properly check for nil before popping an item from the container. Now it works well.

Also, the coroutines in Go "eat" the original stack trace if there's a panic. This is because it just panic()s again from a different place if seq() panic. So, I added the following code to capture the stack trace:

	defer func() {
			if r := recover(); r != nil {
				st := debug.Stack()
				panic(fmt.Sprintf("panic %v in server iterator: %s", r, st))
			}
		}()

@GiedriusS GiedriusS force-pushed the expanded_postings_cache_receiver branch from 5d1ff7b to 15e918f Compare November 19, 2024 10:22
@GiedriusS GiedriusS marked this pull request as ready for review November 19, 2024 10:22
@GiedriusS GiedriusS force-pushed the expanded_postings_cache_receiver branch 2 times, most recently from 1415665 to 5b2d2e8 Compare November 19, 2024 11:12
Port expanded postings cache from Cortex. Huge kudos to @alanprot for
the implementation. I added a TODO item to convert our whole internal
caching infra to be promise based.

Signed-off-by: Giedrius Statkevičius <[email protected]>
@GiedriusS GiedriusS force-pushed the expanded_postings_cache_receiver branch from 5b2d2e8 to 0e7fd72 Compare November 19, 2024 11:44
@GiedriusS GiedriusS merged commit b144eba into main Nov 21, 2024
22 checks passed
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.

3 participants