Skip to content

Remote: Fix a bug that outputs of actions tagged with no-remote are u…#15212

Closed
coeuvre wants to merge 5 commits intobazelbuild:masterfrom
coeuvre:fix-14900
Closed

Remote: Fix a bug that outputs of actions tagged with no-remote are u…#15212
coeuvre wants to merge 5 commits intobazelbuild:masterfrom
coeuvre:fix-14900

Conversation

@coeuvre
Copy link
Copy Markdown
Member

@coeuvre coeuvre commented Apr 11, 2022

…ploaded to remote cache when remote execution is enabled.

Fixes #14900.

Also fixes an issue that action result from just remotely executed action is not saved to disk cache. The root cause is the action result is inlined in the execution response hence not downloaded through remote cache, hence not saved to disk cache. This results in the second build misses the disk cache, but it can still hit the remote cache and fill the disk cache. The third build can hit disk cache.

…ploaded to remote cache when remote execution is enabled.
@coeuvre coeuvre requested a review from a team as a code owner April 11, 2022 15:56
@coeuvre coeuvre requested a review from tjgq April 11, 2022 15:59

/** Returns the {@link Type} of the context. */
Type getType();
private final Spawn spawn;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mark as @Nullable?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

private Step step;

public RemoteActionExecutionContext(
Spawn spawn, RequestMetadata requestMetadata, NetworkTime networkTime) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also - should we make the ctor private to enforce construction through one of the create() methods?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

private final Spawn spawn;
private final SpawnExecutionContext spawnExecutionContext;
private final RemoteActionExecutionContext remoteActionExecutionContext;
private final RemoteActionExecutionContext context;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it a good idea to rename this, given that there are two different context objects?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes. Revert the rename. Extracted RemoteAction to an upper level.

@sgowroji sgowroji added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Apr 12, 2022
@bazel-io bazel-io closed this in 591dcc4 Apr 13, 2022
coeuvre added a commit to coeuvre/bazel that referenced this pull request May 10, 2022
...ploaded to remote cache when remote execution is enabled.

Fixes bazelbuild#14900.

Also fixes an issue that action result from just remotely executed action is not saved to disk cache. The root cause is the action result is inlined in the execution response hence not downloaded through remote cache, hence not saved to disk cache. This results in the second build misses the disk cache, but it can still hit the remote cache and fill the disk cache. The third build can hit disk cache.

Closes bazelbuild#15212.

PiperOrigin-RevId: 441426469
coeuvre added a commit to coeuvre/bazel that referenced this pull request May 27, 2022
…ploaded to remote cache when remote execution is enabled.

Fixes bazelbuild#14900.

Also fixes an issue that action result from just remotely executed action is not saved to disk cache. The root cause is the action result is inlined in the execution response hence not downloaded through remote cache, hence not saved to disk cache. This results in the second build misses the disk cache, but it can still hit the remote cache and fill the disk cache. The third build can hit disk cache.

Closes bazelbuild#15212.

PiperOrigin-RevId: 441426469
ckolli5 added a commit that referenced this pull request May 27, 2022
… are u... (#15453)

* Remote: Fix a bug that outputs of actions tagged with no-remote are u…

…ploaded to remote cache when remote execution is enabled.

Fixes #14900.

Also fixes an issue that action result from just remotely executed action is not saved to disk cache. The root cause is the action result is inlined in the execution response hence not downloaded through remote cache, hence not saved to disk cache. This results in the second build misses the disk cache, but it can still hit the remote cache and fill the disk cache. The third build can hit disk cache.

Closes #15212.

PiperOrigin-RevId: 441426469

* Fix test

Co-authored-by: Chenchu K <ckolli@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team-Remote-Exec Issues and PRs for the Execution (Remote) team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bazel uploads outputs of no-remote actions when used in combination with a disk-cache

3 participants