You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "mridulm (via GitHub)" <gi...@apache.org> on 2023/10/22 09:29:14 UTC
[PR] CELEBORN-1071: Ensure guardedBy is satisfied, fix DCL bugs as well [incubator-celeborn]
mridulm opened a new pull request, #2018:
URL: https://github.com/apache/incubator-celeborn/pull/2018
### What changes were proposed in this pull request?
Ensure appropriate lock is held when accessing/mutating state - as marked with `GuardedBy`.
More [here](https://errorprone.info/bugpattern/GuardedBy).
This also fixes [DCL](https://errorprone.info/bugpattern/DoubleCheckedLocking) bugs observed.
### Why are the changes needed?
Fix bug with locking as identified by error-prone
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Unit tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CELEBORN-1071: Ensure guardedBy is satisfied, fix DCL bugs as well [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2018:
URL: https://github.com/apache/incubator-celeborn/pull/2018#discussion_r1367954422
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/storage/FileWriter.java:
##########
@@ -152,25 +152,26 @@ public void decrementPendingWrites() {
numPendingWrites.decrementAndGet();
}
- @GuardedBy("flushLock")
protected void flush(boolean finalFlush) throws IOException {
- // flushBuffer == null here means writer already closed
- if (flushBuffer != null) {
- int numBytes = flushBuffer.readableBytes();
- if (numBytes != 0) {
- notifier.checkException();
- notifier.numPendingFlushes.incrementAndGet();
- FlushTask task = null;
- if (channel != null) {
- task = new LocalFlushTask(flushBuffer, channel, notifier);
- } else if (fileInfo.isHdfs()) {
- task = new HdfsFlushTask(flushBuffer, fileInfo.getHdfsPath(), notifier);
- }
- addTask(task);
- flushBuffer = null;
- fileInfo.updateBytesFlushed(numBytes);
- if (!finalFlush) {
- takeBuffer();
+ synchronized (flushLock) {
Review Comment:
There are a few locations where `flushLock` is not held when invoking `flush` - which is why error-prone ended up flagging it.
For example:
* `ReducePartitionFileWriter.flush`
* Test cases.
Note, it is possible if we keep walking up the current call stack, it is held :-) But this is subject to evolution anyway - and the additional `synchronized` for an uncontended lock is very cheap.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CELEBORN-1071: Ensure guardedBy is satisfied, fix DCL bugs as well [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2018:
URL: https://github.com/apache/incubator-celeborn/pull/2018#discussion_r1367954422
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/storage/FileWriter.java:
##########
@@ -152,25 +152,26 @@ public void decrementPendingWrites() {
numPendingWrites.decrementAndGet();
}
- @GuardedBy("flushLock")
protected void flush(boolean finalFlush) throws IOException {
- // flushBuffer == null here means writer already closed
- if (flushBuffer != null) {
- int numBytes = flushBuffer.readableBytes();
- if (numBytes != 0) {
- notifier.checkException();
- notifier.numPendingFlushes.incrementAndGet();
- FlushTask task = null;
- if (channel != null) {
- task = new LocalFlushTask(flushBuffer, channel, notifier);
- } else if (fileInfo.isHdfs()) {
- task = new HdfsFlushTask(flushBuffer, fileInfo.getHdfsPath(), notifier);
- }
- addTask(task);
- flushBuffer = null;
- fileInfo.updateBytesFlushed(numBytes);
- if (!finalFlush) {
- takeBuffer();
+ synchronized (flushLock) {
Review Comment:
There are a few locations where `flushLock` is not held when invoking `flush` - which is why error-prone ended up flagging it.
For example:
* `ReducePartitionFileWriter.flush`
* Test cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CELEBORN-1071: Ensure guardedBy is satisfied, fix DCL bugs as well [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #2018: CELEBORN-1071: Ensure guardedBy is satisfied, fix DCL bugs as well
URL: https://github.com/apache/incubator-celeborn/pull/2018
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CELEBORN-1071: Ensure guardedBy is satisfied, fix DCL bugs as well [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on PR #2018:
URL: https://github.com/apache/incubator-celeborn/pull/2018#issuecomment-1774108005
cc @RexXiong @FMX please also take a look at this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CELEBORN-1071: Ensure guardedBy is satisfied, fix DCL bugs as well [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #2018:
URL: https://github.com/apache/incubator-celeborn/pull/2018#discussion_r1368365396
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/storage/FileWriter.java:
##########
@@ -152,25 +152,26 @@ public void decrementPendingWrites() {
numPendingWrites.decrementAndGet();
}
- @GuardedBy("flushLock")
protected void flush(boolean finalFlush) throws IOException {
- // flushBuffer == null here means writer already closed
- if (flushBuffer != null) {
- int numBytes = flushBuffer.readableBytes();
- if (numBytes != 0) {
- notifier.checkException();
- notifier.numPendingFlushes.incrementAndGet();
- FlushTask task = null;
- if (channel != null) {
- task = new LocalFlushTask(flushBuffer, channel, notifier);
- } else if (fileInfo.isHdfs()) {
- task = new HdfsFlushTask(flushBuffer, fileInfo.getHdfsPath(), notifier);
- }
- addTask(task);
- flushBuffer = null;
- fileInfo.updateBytesFlushed(numBytes);
- if (!finalFlush) {
- takeBuffer();
+ synchronized (flushLock) {
Review Comment:
Yeah, I checked locations where `flush` is invoked without synchronizing `flushLock` and confirmed that there wasn't concurrent bug, but I DO agree with your refactor since it's more robust against future evolution :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CELEBORN-1071: Ensure guardedBy is satisfied, fix DCL bugs as well [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2018:
URL: https://github.com/apache/incubator-celeborn/pull/2018#issuecomment-1774046697
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2018?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#2018](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2018?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (17814e3) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/f93be45506d3ec4a0aa2b4cfb9327a05926071cd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (f93be45) will **increase** coverage by `0.10%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #2018 +/- ##
==========================================
+ Coverage 46.64% 46.73% +0.10%
==========================================
Files 165 165
Lines 10531 10531
Branches 959 959
==========================================
+ Hits 4911 4921 +10
+ Misses 5295 5286 -9
+ Partials 325 324 -1
```
[see 2 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2018/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CELEBORN-1071: Ensure guardedBy is satisfied, fix DCL bugs as well [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #2018:
URL: https://github.com/apache/incubator-celeborn/pull/2018#discussion_r1368365396
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/storage/FileWriter.java:
##########
@@ -152,25 +152,26 @@ public void decrementPendingWrites() {
numPendingWrites.decrementAndGet();
}
- @GuardedBy("flushLock")
protected void flush(boolean finalFlush) throws IOException {
- // flushBuffer == null here means writer already closed
- if (flushBuffer != null) {
- int numBytes = flushBuffer.readableBytes();
- if (numBytes != 0) {
- notifier.checkException();
- notifier.numPendingFlushes.incrementAndGet();
- FlushTask task = null;
- if (channel != null) {
- task = new LocalFlushTask(flushBuffer, channel, notifier);
- } else if (fileInfo.isHdfs()) {
- task = new HdfsFlushTask(flushBuffer, fileInfo.getHdfsPath(), notifier);
- }
- addTask(task);
- flushBuffer = null;
- fileInfo.updateBytesFlushed(numBytes);
- if (!finalFlush) {
- takeBuffer();
+ synchronized (flushLock) {
Review Comment:
Yeah, I checked locations where `flush` is invoked without synchronizing `flushLock` and confirmed that there wasn't concurrent bug, but I do agree with your refactor since it's more robust against future evolution :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CELEBORN-1071: Ensure guardedBy is satisfied, fix DCL bugs as well [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture commented on code in PR #2018:
URL: https://github.com/apache/incubator-celeborn/pull/2018#discussion_r1367910965
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/storage/FileWriter.java:
##########
@@ -152,25 +152,26 @@ public void decrementPendingWrites() {
numPendingWrites.decrementAndGet();
}
- @GuardedBy("flushLock")
protected void flush(boolean finalFlush) throws IOException {
- // flushBuffer == null here means writer already closed
- if (flushBuffer != null) {
- int numBytes = flushBuffer.readableBytes();
- if (numBytes != 0) {
- notifier.checkException();
- notifier.numPendingFlushes.incrementAndGet();
- FlushTask task = null;
- if (channel != null) {
- task = new LocalFlushTask(flushBuffer, channel, notifier);
- } else if (fileInfo.isHdfs()) {
- task = new HdfsFlushTask(flushBuffer, fileInfo.getHdfsPath(), notifier);
- }
- addTask(task);
- flushBuffer = null;
- fileInfo.updateBytesFlushed(numBytes);
- if (!finalFlush) {
- takeBuffer();
+ synchronized (flushLock) {
Review Comment:
In caller site this method is inside `synchonized(flushLock)` block, so I think previously there is no concurrent bug here.
##########
worker/src/main/java/org/apache/celeborn/service/deploy/worker/storage/FileWriter.java:
##########
@@ -372,7 +373,9 @@ protected void takeBuffer() {
}
// real action
- flushBuffer = flusher.takeBuffer();
+ synchronized (flushLock) {
Review Comment:
Caller site of `takeBuffer()` is already inside `flushLock`'s synchronization block, so I think previously there is no concurrent bug here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org