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

refactor(s3stream): automate logIdent injection via LogContext #2379

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

misi0202
Copy link

Using com.Automq.Stream.Utils.LogContext for com.Automq.Stream.S3.S3Stream logIdent automatic injection in prefix

Summary of testing strategy (including rationale)
Test the log output format in S3Stream

Committer Checklist (excluded from commit message)

  • [✅] Verify design and implementation
  • [✅] Verify test coverage and CI build status

@CLAassistant
Copy link

CLAassistant commented Mar 20, 2025

CLA assistant check
All committers have signed the CLA.

@misi0202 misi0202 closed this Mar 20, 2025
@misi0202 misi0202 reopened this Mar 21, 2025
@Chillax-0v0
Copy link
Contributor

Hi @misi0202,

I noticed your workflow for requesting changes on PRs and wanted to share a GitHub best practice that might help improve collaboration. When a pull request needs revisions, converting it to a draft (rather than closing it) is generally recommended.

This approach preserves the conversation history and context while clearly indicating the work-in-progress status to collaborators. It also avoids generating multiple closed/reopened event notifications, which can sometimes cause confusion in tracking changes.

The draft feature was introduced specifically for this purpose - you can easily convert PRs using the right sidebar dropdown. When ready, it can be marked "Ready for review" again without losing any context.

Just sharing this as a friendly suggestion – feel free to ignore if you have a specific reason for your current approach!

@misi0202 misi0202 marked this pull request as draft March 21, 2025 08:00
@misi0202
Copy link
Author

Hi @misi0202,

I noticed your workflow for requesting changes on PRs and wanted to share a GitHub best practice that might help improve collaboration. When a pull request needs revisions, converting it to a draft (rather than closing it) is generally recommended.

This approach preserves the conversation history and context while clearly indicating the work-in-progress status to collaborators. It also avoids generating multiple closed/reopened event notifications, which can sometimes cause confusion in tracking changes.

The draft feature was introduced specifically for this purpose - you can easily convert PRs using the right sidebar dropdown. When ready, it can be marked "Ready for review" again without losing any context.

Just sharing this as a friendly suggestion – feel free to ignore if you have a specific reason for your current approach!

Thanks for your suggestion,I have changed this PR to draft and I did modify two files by mistake.

@misi0202 misi0202 marked this pull request as ready for review March 21, 2025 08:35
@misi0202 misi0202 marked this pull request as draft March 21, 2025 08:36
@misi0202
Copy link
Author

494524d32e6c5f00c3e35f4d6f1ab82
@Chillax-0v0 After I committed the changes to change the mistakes, bulid S3stream in E2E-TEST for AutoMQ S3Stream encountered the following error message. Shown as automq/stream/s3 / operator/TrafficVolumeLimiter.java appear problem, whether this modified code with me, thank you for your guidance and reply.

@Chillax-0v0
Copy link
Contributor

494524d32e6c5f00c3e35f4d6f1ab82 @Chillax-0v0 After I committed the changes to change the mistakes, bulid S3stream in E2E-TEST for AutoMQ S3Stream encountered the following error message. Shown as automq/stream/s3 / operator/TrafficVolumeLimiter.java appear problem, whether this modified code with me, thank you for your guidance and reply.

Apologies for the confusion. The S3Stream E2E Test has been recently broken, and this error is unrelated to your changes in this PR.
You can safely ignore this error message - the specific Action for S3Stream E2E Test won't be triggered before merging anyway. Thank you for your contribution and for double-checking!

@misi0202 misi0202 marked this pull request as ready for review March 21, 2025 09:02
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.

3 participants