Always use datetime.now(timezone.utc) in paasta status #3963
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will make it clear that we're always using UTC internally and avoids the footgun of using utcnow() I've verified that the output for warming up pods is still the same in both this version of the code as well as in status quo - as well as the same for flink and kafka workloads
nemacysts
force-pushed
the
luisp/PAASTA-18160-tz
branch
from
8bf156a
to
629e89b
Compare
This is probably better than the previous output where it was unclear what offset the times were in without looking at the parenthentical humanized time
nemacysts
commented
Sep 18, 2024
| @@ -730,7 +733,7 @@ def get_pod_uptime(pod_deployed_timestamp: str): | |||
|
|
|||
|
|
|||
| def append_pod_status(pod_status, output: List[str]): | |||
| output.append(f" Pods:") | |||
| output.append(" Pods:") | |||
There was a problem hiding this comment.
i can revert these f-string changes - but i noticed that these didn't need to be f-strings while tracing the impact of my changes
| @@ -1825,7 +1832,7 @@ def print_kafka_status( | |||
| desired_state = annotations.get(paasta_prefixed("desired_state")) | |||
| if desired_state is None: | |||
| raise ValueError( | |||
| f"expected desired state in kafka annotation, but received none" | |||
| "expected desired state in kafka annotation, but received none" | |||
There was a problem hiding this comment.
same as above: i can revert these f-string changes - but i noticed that these didn't need to be f-strings while tracing the impact of my changes
| start_time = datetime.strptime( | ||
| broker["deployed_timestamp"], "%Y-%m-%dT%H:%M:%SZ" | ||
| ) | ||
| delta = datetime.utcnow() - start_time | ||
| ).replace(tzinfo=timezone.utc) |
There was a problem hiding this comment.
this will add a +00:00 to broker started times - e.g., we'll go from
Started
2024-09-18 01:11:36 (20 hours ago)
to
Started
2024-09-18 01:11:36+00:00 (20 hours ago)
but imo, this is a net positive since without looking at the parenthetical, it's entirely unclear what TZ is being used
jfongatyelp
approved these changes
Sep 19, 2024
nleigh
approved these changes
Sep 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will make it clear that we're always using UTC internally and avoids the footgun of using utcnow()
I've verified that the output for warming up pods is still the same in both this version of the code as well as in status quo - as well as the same for flink and kafka workloads
NOTE: there's still usages of utcnow or now() without an explicit timezone - but that's for another day