-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
replaced crypto/sha with https://github.com/minio/sha256-simd #5738
Conversation
Signed-off-by: utukj <utukphd@gmail.com>
internal/cortex/chunk/schema_util.go
Outdated
@@ -13,6 +12,8 @@ import ( | |||
"strings" | |||
"sync" | |||
|
|||
"github.com/minio/sha256-simd" | |||
|
|||
"fmt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you group the imports?
pkg/block/metadata/hash.go
Outdated
"encoding/hex" | ||
"fmt" | ||
"io" | ||
"os" | ||
"path/filepath" | ||
|
||
"github.com/minio/sha256-simd" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto, let's merge this with the section starting from line 15
pkg/reloader/reloader.go
Outdated
// - Watch on changes against certain file e.g (`cfgFile`). | ||
// - Optionally, specify different output file for watched `cfgFile` (`cfgOutputFile`). | ||
// This will also try decompress the `cfgFile` if needed and substitute ALL the envvars using Kubernetes substitution format: (`$(var)`) | ||
// - Watch on changes against certain directories (`watchedDirs`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm, why these are changed? Let's update them back
pkg/reloader/reloader.go
Outdated
@@ -69,6 +68,8 @@ import ( | |||
"sync" | |||
"time" | |||
|
|||
"github.com/minio/sha256-simd" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same grouping issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops. I think a go fmt extension did that.
Signed-off-by: utukj <utukphd@gmail.com>
e6201e7
to
6c73ea0
Compare
I know we talked about testing this with other OS-es: #5722 (comment), however testing manually anything is not sustainable. If our CI is green, we should merge it. If image build is failing later on on main, we should add extra check for that in the PR CI - it's not sustainable for community to manually test things like that, especially as likely no one runs Thanos on freeBSD or ppc64le and we don't claim full support for that (yet). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
For future - we should benchmark if this extra deps was even worth it 🙈 |
cc #5764 |
…-io#5738) * replaced crypto/sha with https://github.com/minio/sha256-simd Signed-off-by: utukj <utukphd@gmail.com> * fixed import grouping Signed-off-by: utukj <utukphd@gmail.com> Signed-off-by: utukj <utukphd@gmail.com>
Is a cryptographically-strong digest really needed for this use case? If the goal is just to have a high quality but also fast hash, maybe it makes sense to look at https://cyan4973.github.io/xxHash/. |
Signed-off-by: utukj utukphd@gmail.com
Changes
Replaced crypto/sha package with https://github.com/minio/sha256-simd
Verification