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

replaced crypto/sha with https://github.com/minio/sha256-simd #5738

Merged
merged 2 commits into from Sep 30, 2022

Conversation

utukJ
Copy link
Contributor

@utukJ utukJ commented Sep 28, 2022

Signed-off-by: utukj utukphd@gmail.com

  • Change is not relevant to the end user.

Changes

Replaced crypto/sha package with https://github.com/minio/sha256-simd

Verification

Signed-off-by: utukj <utukphd@gmail.com>
@@ -13,6 +12,8 @@ import (
"strings"
"sync"

"github.com/minio/sha256-simd"

"fmt"
Copy link
Contributor

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?

"encoding/hex"
"fmt"
"io"
"os"
"path/filepath"

"github.com/minio/sha256-simd"
Copy link
Contributor

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

// - 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`).
Copy link
Contributor

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

@@ -69,6 +68,8 @@ import (
"sync"
"time"

"github.com/minio/sha256-simd"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same grouping issue

Copy link
Contributor Author

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>
@bwplotka
Copy link
Member

Flake, so restarted CI job:
image

@bwplotka
Copy link
Member

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).

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.

LGTM, thanks!

@bwplotka bwplotka enabled auto-merge (squash) September 30, 2022 11:42
@bwplotka bwplotka merged commit 6f86ada into thanos-io:main Sep 30, 2022
@bwplotka
Copy link
Member

bwplotka commented Oct 6, 2022

For future - we should benchmark if this extra deps was even worth it 🙈

@bwplotka
Copy link
Member

bwplotka commented Oct 6, 2022

cc #5764

utukJ added a commit to utukJ/thanos that referenced this pull request Oct 13, 2022
…-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>
@dswarbrick
Copy link

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/.

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.

None yet

4 participants