Skip to content

Conversation

@mrnugget
Copy link
Contributor

This fixes the regression reported in
https://github.com/sourcegraph/sourcegraph/issues/21230 and introduced
by yours truly in
d6c876c.

With the introduction of the Coordinator and the explicit steps of
checking-the-cache-for and building-of ChangesetSpecs outside the
executor, the TaskStatus.ChangesetSpecs field wasn't set at the time
when FinishedAt was set.

The batchProgressPrinter assumed, though, that this was the case and
that if FinishedAt was set that len(taskStatus.ChangesetSpecs) == 0
means "No changes".

This change here fixes the problem by distinguishing between the two
states: finished execution of steps and finished building changeset
specs.

The problem is that it's still a slight regression in behaviour:
previously the diff stats would be printed in the status bar and in the
verbose mode as tasks were finishing.

Now that we build all changeset specs at once, after all of them have
been executed, we can't update the status bar to include diff stats and
the verbose messages will be logged all at once.

I still think that the current code (with the Coordinator) is better
than what we had before and the hard problem here is fixed (no wrong
information being displayed), but longer term I think there's a solution
possible in which we decouple the task execution and its status much
more and make it possible to build better UIs for the status of
execution.

That I think should be approached separately though.

This fixes the regression reported in
https://github.com/sourcegraph/sourcegraph/issues/21230 and introduced
by yours truly in
d6c876c.

With the introduction of the Coordinator and the explicit steps of
checking-the-cache-for and building-of ChangesetSpecs outside the
executor, the `TaskStatus.ChangesetSpecs` field wasn't set at the time
when `FinishedAt` was set.

The `batchProgressPrinter` assumed, though, that this was the case and
that if `FinishedAt` was set that `len(taskStatus.ChangesetSpecs) == 0`
means "No changes".

This change here fixes the problem by distinguishing between the two
states: finished execution of steps and finished building changeset
specs.

The problem is that it's still a slight regression in behaviour:
previously the diff stats would be printed in the status bar and in the
verbose mode *as tasks were finishing*.

Now that we build all changeset specs at once, after all of them have
been executed, we can't update the status bar to include diff stats and
the verbose messages will be logged all at once.

I still think that the current code (with the Coordinator) is better
than what we had before and the hard problem here is fixed (no wrong
information being displayed), but longer term I think there's a solution
possible in which we decouple the task execution and its status much
more and make it possible to build better UIs for the status of
execution.

That I think should be approached separately though.
@mrnugget mrnugget requested a review from a team May 21, 2021 09:48
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

Seems sensible to me. We can always improve here. Also, a UI for this should make much of it obsolete anyways

@mrnugget
Copy link
Contributor Author

Also, a UI for this should make much of it obsolete anyways

Yeah, except a better way to communicate status 😐

@mrnugget mrnugget merged commit 348c49a into main May 21, 2021
@mrnugget mrnugget deleted the mrn/fix-verbose-regression branch May 21, 2021 12:05
scjohns pushed a commit that referenced this pull request Apr 24, 2023
This fixes the regression reported in
https://github.com/sourcegraph/sourcegraph/issues/21230 and introduced
by yours truly in
d6c876c.

With the introduction of the Coordinator and the explicit steps of
checking-the-cache-for and building-of ChangesetSpecs outside the
executor, the `TaskStatus.ChangesetSpecs` field wasn't set at the time
when `FinishedAt` was set.

The `batchProgressPrinter` assumed, though, that this was the case and
that if `FinishedAt` was set that `len(taskStatus.ChangesetSpecs) == 0`
means "No changes".

This change here fixes the problem by distinguishing between the two
states: finished execution of steps and finished building changeset
specs.

The problem is that it's still a slight regression in behaviour:
previously the diff stats would be printed in the status bar and in the
verbose mode *as tasks were finishing*.

Now that we build all changeset specs at once, after all of them have
been executed, we can't update the status bar to include diff stats and
the verbose messages will be logged all at once.

I still think that the current code (with the Coordinator) is better
than what we had before and the hard problem here is fixed (no wrong
information being displayed), but longer term I think there's a solution
possible in which we decouple the task execution and its status much
more and make it possible to build better UIs for the status of
execution.

That I think should be approached separately though.
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.

3 participants