-
Notifications
You must be signed in to change notification settings - Fork 9
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
🐛 Waypoint wait for consumer to be ready #1331
base: master
Are you sure you want to change the base?
Conversation
f56100e
to
3da632f
Compare
K8s Regression Test Coverage
|
waypoint/services/nats_service.py
Outdated
not_ready = True | ||
while not_ready: | ||
consumer_info = await subscription.consumer_info() | ||
if isinstance(consumer_info, ConsumerInfo): | ||
bound_logger.trace( | ||
"Consumer is ready {}, {}", | ||
consumer_info.name, | ||
consumer_info.stream_name, | ||
) | ||
not_ready = False |
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.
I can't figure out from the nats.py code if this actually ensures the consumer is ready. ConsumerInfo has:
@dataclass
class ConsumerInfo(Base):
"""
ConsumerInfo represents the info about the consumer.
"""
name: str
stream_name: str
config: ConsumerConfig
# FIXME: Do not handle dates for now.
# created: datetime
delivered: Optional[SequenceInfo] = None
ack_floor: Optional[SequenceInfo] = None
num_ack_pending: Optional[int] = None
num_redelivered: Optional[int] = None
num_waiting: Optional[int] = None
num_pending: Optional[int] = None
cluster: Optional[ClusterInfo] = None
push_bound: Optional[bool] = None
and none of it stands out as something we can check that it's ready. So we just assume if we get this response, it's ready. Only way to confirm that is if it fixes the bug ... so, let me know if it does
One nitpick is that I prefer: ready = False
and while not ready
, then in loop ready = True
.
I think the if isinstance check can be dropped.
Come to think about it, the while loop is probably redundant, because the await can only return ConsumerInfo, or raise exception. So I don't think it'll ever loop.
So, presumably we just wanna get consumer info before proceeding.
K8s Test Coverage
|
K8s Regression Test Coverage
|
c12cfc3
to
b215320
Compare
b215320
to
1f588ac
Compare
K8s Test Coverage
|
K8s Regression Test Coverage
|
K8s Test Coverage
|
K8s Regression Test Coverage
|
1f588ac
to
ea0a902
Compare
K8s Test Coverage
|
K8s Regression Test Coverage
|
K8s Test Coverage
|
K8s Regression Test Coverage
|
|
@@ -93,6 +96,11 @@ | |||
subscription = await self.js_context.pull_subscribe( | |||
config=config, **subscribe_kwargs | |||
) | |||
|
|||
# Get consumer info to verify that the subscription was successful | |||
# TODO test if this is necessary/helps |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
TODO test if this is necessary/helps Note
Getting
consumer_info
on NATS subscription before starting to pull events to make sure consumer is ready.This solves an issue where we saw timeouts on NATS subscriptions
Method suggested by Claude/GPT/Copilot