Skip to content
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

rust: add more rust logging #351

Merged
merged 5 commits into from
Apr 12, 2024
Merged

rust: add more rust logging #351

merged 5 commits into from
Apr 12, 2024

Conversation

dbanda
Copy link
Contributor

@dbanda dbanda commented Apr 9, 2024

Adds more logging to rust to more closely match python. Specifically, we add logging to show when the processor is terminating or closing/rejoining strategies.

We also introduce logging to show the member id as supplied by the broker. This is useful for debugging issue related to rebalancing in kafka since the broker logs only show member id.

@dbanda dbanda requested review from a team as code owners April 9, 2024 20:29
@dbanda dbanda requested a review from a team April 9, 2024 20:29
arroyo/backends/kafka/consumer.py Show resolved Hide resolved
arroyo/processing/processor.py Show resolved Hide resolved
@@ -505,6 +515,7 @@ mod tests {
impl AssignmentCallbacks for EmptyCallbacks {
fn on_assign(&self, partitions: HashMap<Partition, u64>) {
println!("assignment event: {:?}", partitions);

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@@ -628,6 +628,11 @@ def close(self, timeout: Optional[float] = None) -> None:
def closed(self) -> bool:
return self.__state is KafkaConsumerState.CLOSED

@property
def member_id(self) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should also add @cached_property, depending on the cost?

Suggested change
def member_id(self) -> str:
@cached_property
def member_id(self) -> str:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it cheap. Also, I believe the member id changes depending on the rebalancing strategy used. So caching might be inacurate.

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

not too happy to see unsafe and unwrap, but I guess rdkafka does not provide a safe function for this?

@dbanda dbanda merged commit 6df76c8 into main Apr 12, 2024
11 checks passed
@dbanda dbanda deleted the dbanda/add-more-rust-logging branch April 12, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants