Feat: new cleaning rule to orphaned subscriptions - #39538
Conversation
…red orphaned subscriptions of taxirides topic
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
HansMarcus01
changed the title
|
Assigning reviewers: R: @shunping added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
can you upload the output from the dry run of your changes? so we can see expected results |
| if not self.prefixes or any(subscription_name.startswith(f"{self.project_path}/subscriptions/{prefix}") for prefix in self.prefixes): | ||
| # Check if the subscription has a topic associated with it | ||
| if subscription.detached: | ||
| # Safe orphan detection: |
There was a problem hiding this comment.
we need to be thoughtful about the behavior of this function.
The behavior before this change is:
- No prefixes are defined, so iterate through ALL subscriptions, and capture only detached ones. Is that in line with your understanding?
If we apply this change, we:
- No longer catch all detached subscriptions because prefixes are defined. Does that make sense?
- We are also adding a special case
if taxi...which should be covered by prefixes. This is turns the code into a totally strange thing.
So we have to change this function to something like this:
for s in list_subscriptions:
if subscription.detached = true:
d[s....]...
elif any(subscription_name.startswith(f"{self.project_path}/subscriptions/{prefix}") for prefix in self.prefixes):
d[s...]...
this covers our cases properly. Please go ahead and make that change.
|
|
||
| def test_active_resources(self): | ||
| """Test _active_resources method.""" | ||
| # Keep the existing standard prefix coverage and add the taxi-only cleanup prefix. |
There was a problem hiding this comment.
can you confirm that you ran this test? also, mark one sub with detached=True to make sure that case is covered.
…d active subscriptions under prefixes.
| def _active_resources(self) -> dict: | ||
| d = {} | ||
| self.client = pubsub_v1.SubscriberClient() | ||
| taxi_prefix = f"{self.project_path}/subscriptions/taxirides-realtime_beam_" |
There was a problem hiding this comment.
why are you adding the taxi prefix here? the taxi prefix should be included in self.prefixes, so it doesn't make sense to re-check it.
| subscription_name.startswith(f"{self.project_path}/subscriptions/{prefix}") for prefix in self.prefixes | ||
| ) and subscription_name.startswith(taxi_prefix): | ||
| d[subscription_name] = GoogleCloudResource(resource_name=subscription_name, clock=self.clock) |
There was a problem hiding this comment.
why do we check self.prefixes AND taxi_prefix? do you see how that does not make sense?
2 participants
This pull request introduces a new cleanup rule that identifies orphaned subscriptions for the topic related to taxirides.
Problem detected
taxirides-realtime_beam_-...was identified, connected to the public, continuous New York taxi topic.Solution
stale_cleaner.py:PubSubSubscriptionCleanerwas modified to evaluate and mark for deletion subscriptions with the prefixtaxirides-realtime_beam_that are older than 24 hours, even if they are not in thedetachedstate (since the NY taxi topic is permanent and public).test_stale_cleaner.py: Unit tests were added to validate this specific taxi-related criterion in isolation, ensuring that other subscriptions remain intact and safe.The document describing the justification for cleaning this resource can be found at the following link: https://gist.github.com/HansMarcus01/101c53d0e9fa6179a5f631b59324df96
Output obtained
View test script and execution output
Command used: