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

Use 32bit number for index cache init so Thanos will build on 32bit OS #2218

Closed
wants to merge 1 commit into from

Conversation

slim-bean
Copy link

@slim-bean slim-bean commented Mar 5, 2020

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Previously the index cache was initialized with math.MaxInt64 however this will cause Thanos to fail to compile for a 32bit system (Raspberry Pi), It seems a reasonable compromise here is to init the cache with math.MaxInt32 which addresses the issue. This does however change the cache to hold 2^32 objects vs 2^64, though I think this should still be plenty??

Another alternative would be something like this:

l, err := lru.NewLRU(int(int64(math.MaxInt64)), c.onEvict)

However this kind of feels worse to me unless there is an expectation to have more than ~4billion entries in this cache?

Relevant Go issue: golang/go#23086

Signed-off-by: Edward Welch edward.welch@grafana.com

Signed-off-by: Edward Welch <edward.welch@grafana.com>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

I think this is alright, but fix like this is a droplet in the ocean of 64MaxInts we used everywhere else (:

So we need to decide what we do with that everywhere and then also add 32 bit builds to cross-build GithuAction job if we agree on this. (: What's your motivation for this?

@slim-bean
Copy link
Author

slim-bean commented Mar 13, 2020

I ran into this building cortex for arm (and I believe it will also affect Loki arm builds now too I think @pstibrany opened an issue to fix this as well).

I run all of this stuff on raspberry pi's in one form or another and the defacto operating system (raspbian) is 32bit.

This was the only instance failing to compile for cortex (I haven't tried to build thanos itself for 32bit arm)

@slim-bean
Copy link
Author

Interesting i think of all the other places that MaxInt64 is used, it must be the case that none of them are passed into a function accepting int as is done in this case. So I guess the issue is not so much the use of MaxInt64, it's using it as an int

#2268 looks like a different solution to the same issue found when trying to update Loki to the latest Cortex before I had a chance to catch @pstibrany and tell him about this PR :)

@pstibrany
Copy link
Contributor

pstibrany commented Mar 13, 2020

Interesting i think of all the other places that MaxInt64 is used, it must be the case that none of them are passed into a function accepting int as is done in this case. So I guess the issue is not so much the use of MaxInt64, it's using it as an int

#2268 looks like a different solution to the same issue found when trying to update Loki to the latest Cortex before I had a chance to catch @pstibrany and tell him about this PR :)

Oh, I haven’t even bothered to check if another issue/pr already exists :( Sorry Ed.

You’re correct that issue is different size of int type on different platforms.

@GiedriusS
Copy link
Member

#2268 has been merged so we can close this, right?

@pstibrany
Copy link
Contributor

#2268 has been merged so we can close this, right?

Yes, it solves the same problem.

@GiedriusS GiedriusS closed this Mar 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants