-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
chore(cli): fix bugs introduced in stack monitor #9830
Conversation
In #9795, I was under the impression that the `DescribeStackEvents` would list events in chronological order. This was a misunderstanding resulting from not reading the documentation-- in fact, the events are returned in reverse chronological order. The paging logic was therefore wrong and the events would be fed into the activity printer in the wrong order, leading to incorrect progress reporting. This is a critical bug and must be fixed immediately! While I was in there, fixed two other paper cuts around the printer at the same time: - Events occurring near the start of the monitor would be missed. This was because time-based filtering would be employed based on the client machine's local clock, which might be out of sync with the remote clock. Use the server's clock instead, by using the `CreationTime` of the ChangeSet as the start time. - Events occurring near the end of the monitor would be missed. This was because `DescribeStackStatus` might return a completion state before the monitor had polled the events that led up to the completion. Fix by doing a final poll after the stop call has been received, so we are guaranteed to have seen all events.
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! One question.
Added the do-not-merge
label if somebody else wants to chime in, but I'm fine with merging this.
@@ -83,7 +96,7 @@ export class StackActivityMonitor { | |||
/** | |||
* Determines which events not to display | |||
*/ | |||
private startTime = Date.now(); | |||
private readonly startTime: number; |
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.
Any reason not to leave this as a Date
?
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.
Just simpler implementation, and since it doesn't affect the public API I prefer this.
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
In #9795, I was under the impression that the
DescribeStackEvents
would list events in chronological order.
This was a misunderstanding resulting from not reading the documentation--
in fact, the events are returned in reverse chronological order.
The paging logic was therefore wrong and the events would be fed into
the activity printer in the wrong order, leading to incorrect progress
reporting.
This is a critical bug and must be fixed immediately!
While I was in there, fixed two other paper cuts around the
printer at the same time:
Events occurring near the start of the monitor would be missed. This
was because time-based filtering would be employed based on the
client machine's local clock, which might be out of sync with the
remote clock. Use the server's clock instead, by using the
CreationTime
of the ChangeSet as the start time.Events occurring near the end of the monitor would be missed. This
was because
DescribeStackStatus
might return a completion statebefore the monitor had polled the events that led up to the completion.
Fix by doing a final poll after the stop call has been received,
so we are guaranteed to have seen all events.
The actual claim in that PR was incorrect. We were not quadratic (we
wouldn't list all pages on every tick), we were just more linear than
necessary (we would list all pages on the first tick... which is still
a lot). The current PR properly fixes the issue by stopping paging
not just if we've already seen the events, but also if the events are
starting to fall outside the time window we are interested in.
This actually fixes #9470.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license