Skip to content

Fix ConsumeDriver running status#748

Merged
wu-sheng merged 3 commits intoapache:mainfrom
JoeCqupt:main
Jan 19, 2025
Merged

Fix ConsumeDriver running status#748
wu-sheng merged 3 commits intoapache:mainfrom
JoeCqupt:main

Conversation

@JoeCqupt
Copy link
Contributor

@JoeCqupt JoeCqupt commented Jan 17, 2025 •
edited
Loading

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
  • Update the CHANGES log.

@wu-sheng
Copy link
Member

wu-sheng commented Jan 17, 2025

Could you explain why do this change?

JoeCqupt
JoeCqupt commented Jan 18, 2025
View reviewed changes
...rier/src/main/java/org/apache/skywalking/apm/commons/datacarrier/consumer/ConsumeDriver.java Outdated
}
lock.lock();
try {
if (running){
Copy link
Contributor Author

@JoeCqupt JoeCqupt Jan 18, 2025

Choose a reason for hiding this comment

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

Double Checked Locking

@JoeCqupt
Copy link
Contributor Author

JoeCqupt commented Jan 18, 2025

ConsumeDriver use a lock + running status to protect begin method. but there is a problem with code. it should use Double Checked Locking

@wu-sheng
Copy link
Member

wu-sheng commented Jan 18, 2025

ConsumeDriver use a lock + running status to protect begin method. but there is a problem with code. it should use Double Checked Locking

Did you face some issues? Could you paste the logs here?
Or just from code review perspective?

@wu-sheng wu-sheng added the bug Something isn't working label Jan 18, 2025
@wu-sheng wu-sheng added this to the 9.4.0 milestone Jan 18, 2025
@wu-sheng
Copy link
Member

wu-sheng commented Jan 18, 2025

You should update changes.md and describe what is fixed.

@wu-sheng
Copy link
Member

wu-sheng commented Jan 18, 2025

You should update changes.md and describe what is fixed.

CI seems strange. Please update this and push again.

@JoeCqupt
Copy link
Contributor Author

JoeCqupt commented Jan 18, 2025

ConsumeDriver use a lock + running status to protect begin method. but there is a problem with code. it should use Double Checked Locking

Did you face some issues? Could you paste the logs here? Or just from code review perspective?

I just found it when I am studying the skywalking-java source code

@wu-sheng
Copy link
Member

wu-sheng commented Jan 18, 2025

It seems you broke the code style, please check before submit.

@wu-sheng wu-sheng merged commit affbaa8 into apache:main Jan 19, 2025
193 of 194 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wu-sheng wu-sheng wu-sheng approved these changes

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

9.4.0

Development

Successfully merging this pull request may close these issues.

2 participants

@JoeCqupt @wu-sheng