Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions .test-infra/tools/stale_cleaner.py
Original file line number Diff line number Diff line change
Expand Up @@ -336,10 +336,13 @@ def _active_resources(self) -> dict:
for subscription in self.client.list_subscriptions(request={"project": self.project_path}):
subscription_name = subscription.name
# Apply prefix filtering if prefixes are defined
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:
if subscription.detached:
d[subscription_name] = GoogleCloudResource(resource_name=subscription_name, clock=self.clock)
#Only attached subscriptions with the NYC taxi prefix are eligible.
elif any(
subscription_name.startswith(f"{self.project_path}/subscriptions/{prefix}") for prefix in self.prefixes
):
d[subscription_name] = GoogleCloudResource(resource_name=subscription_name, clock=self.clock)
Comment on lines +343 to +345

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why do we check self.prefixes AND taxi_prefix? do you see how that does not make sense?


return d

Expand Down Expand Up @@ -416,8 +419,10 @@ def clean_pubsub_subscriptions():
project_id = DEFAULT_PROJECT_ID
bucket_name = DEFAULT_BUCKET_NAME

# No prefixes are defined for subscriptions so we will delete all stale subscriptions
prefixes = []
# Restrict subscription cleanup to the NYC taxi prefix only.
prefixes = [
"taxirides-realtime_beam_",
]

# Create a PubSubSubscriptionCleaner instance
cleaner = PubSubSubscriptionCleaner(project_id=project_id, bucket_name=bucket_name,
Expand Down
62 changes: 45 additions & 17 deletions .test-infra/tools/test_stale_cleaner.py
Original file line number Diff line number Diff line change
Expand Up @@ -431,30 +431,58 @@ def test_init(self):
self.assertEqual(self.cleaner.time_threshold, self.time_threshold)
self.assertIsInstance(self.cleaner.clock, FakeClock)

def test_active_resources(self):
"""Test _active_resources method."""
# Mock subscriptions
sub1 = mock.Mock()
sub1.name = "projects/test-project/subscriptions/test-prefix-sub1"
sub1.topic = "projects/test-project/topics/some-topic"
def test_active_resources_active_subscriptions(self):
"""Valida que las suscripciones activas con el prefijo de taxirides sean identificadas."""
self.cleaner.prefixes = ["taxirides-realtime_beam_"]

sub2 = mock.Mock()
sub2.name = "projects/test-project/subscriptions/test-prefix-sub2-detached"
sub2.topic = "_deleted-topic_"
# Suscripción activa con el prefijo correcto de taxis
sub_taxi_active = mock.Mock()
sub_taxi_active.name = f"projects/{self.project_id}/subscriptions/taxirides-realtime_beam_-12345"
sub_taxi_active.topic = "projects/pubsub-public-data/topics/taxirides-realtime"
sub_taxi_active.detached = False

sub3 = mock.Mock()
sub3.name = "projects/test-project/subscriptions/other-prefix-sub3"
sub3.topic = "projects/test-project/topics/another-topic"
# Suscripción activa con un prefijo distinto
sub_other_active = mock.Mock()
sub_other_active.name = f"projects/{self.project_id}/subscriptions/other-prefix-sub"
sub_other_active.topic = f"projects/{self.project_id}/topics/another-topic"
sub_other_active.detached = False

self.mock_subscriber_client.list_subscriptions.return_value = [sub1, sub2, sub3]
self.mock_subscriber_client.list_subscriptions.return_value = [sub_taxi_active, sub_other_active]

with SilencePrint():
active = self.cleaner._active_resources()

self.assertIn("projects/test-project/subscriptions/test-prefix-sub1", active)
self.assertIn("projects/test-project/subscriptions/test-prefix-sub2-detached", active)
self.assertNotIn("projects/test-project/subscriptions/other-prefix-sub3", active)
self.assertEqual(len(active), 2)
# Verificamos que solo capture la suscripción de taxi, descartando la otra
self.assertIn(sub_taxi_active.name, active)
self.assertNotIn(sub_other_active.name, active)
self.assertEqual(len(active), 1)

def test_active_resources_detached_subscriptions(self):
"""Valida que las suscripciones desconectadas se mantengan en la lista de activos a limpiar,
independientemente de los prefijos específicos de taxis."""
self.cleaner.prefixes = ["test-prefix"]

# Suscripción desconectada (debería incluirse en la recolección)
sub_detached = mock.Mock()
sub_detached.name = f"projects/{self.project_id}/subscriptions/test-prefix-detached"
sub_detached.topic = "_deleted-topic_"
sub_detached.detached = True

# Suscripción conectada normal (debería ignorarse)
sub_attached = mock.Mock()
sub_attached.name = f"projects/{self.project_id}/subscriptions/test-prefix-attached"
sub_attached.topic = f"projects/{self.project_id}/topics/some-topic"
sub_attached.detached = False

self.mock_subscriber_client.list_subscriptions.return_value = [sub_detached, sub_attached]

with SilencePrint():
active = self.cleaner._active_resources()

# Verificamos que solo se registre la suscripción huérfana
self.assertIn(sub_detached.name, active)
self.assertNotIn(sub_attached.name, active)
self.assertEqual(len(active), 1)

def test_delete_resource(self):
"""Test _delete_resource method."""
Expand Down
Loading