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.

Sign up for GitHub

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

Jump to bottom

fix(388): replace zerolog reference with slog #390

Merged
merged 1 commit into from
Nov 29, 2023
Merged

fix(388): replace zerolog reference with slog #390

merged 1 commit into from
Nov 29, 2023

Conversation

lsytj0413
Copy link
Contributor

Fixes: #388

merlimat
merlimat approved these changes Nov 29, 2023
View reviewed changes
Copy link
Collaborator

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

Thanks for the great refactoring!

common/client_pool.go
cp.log.Warn(
"Failed to close GRPC connection",
slog.String("server_address", target),
slog.Any("Error", err),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: should we consider standardizing on error as the key for all error messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes，i name the key of error as Error，and slog didn't provide an convention for it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was meaning: would it be better to use lower cap error to be more consistent to the other log keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, i misunderstand it. I will fix it at next time.

@merlimat merlimat merged commit 0689a1a into streamnative:main Nov 29, 2023
4 checks passed
@lsytj0413 lsytj0413 deleted the mv-slog branch November 30, 2023 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merlimat merlimat merlimat approved these changes

@mattisonchao mattisonchao Awaiting requested review from mattisonchao mattisonchao is a code owner

@coderzc coderzc Awaiting requested review from coderzc coderzc is a code owner

@RobertIndie RobertIndie Awaiting requested review from RobertIndie RobertIndie is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

【log】replace pkg's zerolog reference with slog
2 participants
@lsytj0413 @merlimat