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

Completeness checking store should not check if directory digests exist #748

Merged

Conversation

allada
Copy link
Member

@allada allada commented Mar 10, 2024

REv2 does not require clients to upload directory digests. Instead they are only required to upload the tree root which has all the directories in it.

closes #747


This change is Reviewable

Copy link
Member Author

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04

a discussion (no related file):
Needs tests


REv2 does not require clients to upload directory digests. Instead
they are only required to upload the tree root which has all the
directories in it.

closes TraceMachina#747
Copy link
Member Author

@allada allada left a comment

Choose a reason for hiding this comment

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

+@aaronmondal
+@zbirenbaum

Fyi: @blakehatch

Reviewable status: 0 of 2 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), integration-tests (20.04), macos-13, pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale (waiting on @aaronmondal and @zbirenbaum)

Copy link
Collaborator

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained (waiting on @aaronmondal and @zbirenbaum)


nativelink-store/src/completeness_checking_store.rs line 93 at r2 (raw file):

                .into_iter()
                .chain(tree.root)
                .flat_map(|dir| dir.files.into_iter().filter_map(|f| f.digest.map(DigestInfo::try_from)));

nit: to make this a bit more readable, consider breaking this up into a few lines:

.flat_map(|dir| {
    dir.files
        .par_iter()
        .filter_map(|f| f.digest.map(DigestInfo::try_from))
})

sometimes it can go either way but the.filter_map is a good candidate for being on its own line.

Copy link
Member

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @aaronmondal and @zbirenbaum)

Copy link
Member Author

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained (waiting on @aaronmondal and @zbirenbaum)


nativelink-store/src/completeness_checking_store.rs line 93 at r2 (raw file):

Previously, MarcusSorealheis (Marcus Eagan) wrote…

nit: to make this a bit more readable, consider breaking this up into a few lines:

.flat_map(|dir| {
    dir.files
        .par_iter()
        .filter_map(|f| f.digest.map(DigestInfo::try_from))
})

sometimes it can go either way but the.filter_map is a good candidate for being on its own line.

Sadly it was like you showed but our linter forced it this way. I can use a comment trick to force it into this format, but I would prefer to not have this copied elsewhere.

There's not much I can do here, even temporary variables won't work because it will require it to be a lambda/function... and that would make the code worse imo.

Copy link
Member

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @zbirenbaum)

Copy link
Contributor

@zbirenbaum zbirenbaum left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 2 of 2 LGTMs obtained

Copy link
Member

@blakehatch blakehatch left a comment

Choose a reason for hiding this comment

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

:lgtm: that change makes this section of the code so much simpler 🫠

Reviewable status: 2 of 2 LGTMs obtained

Copy link
Collaborator

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 2 of 2 LGTMs obtained

@aaronmondal aaronmondal merged commit e979e31 into TraceMachina:main Mar 15, 2024
25 checks passed
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.

CompletenessCheckingStore requires all Directory digest values be uploaded to store but not required by spec
6 participants