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/23 16:41:12 UTC
[PR] CELEBORN-1079: Fix use of GuardedBy in client-flink/common [incubator-celeborn]
mridulm opened a new pull request, #2029:
URL: https://github.com/apache/incubator-celeborn/pull/2029
### What changes were proposed in this pull request?
* Fix use of `GuardedBy` on nonexistant lock.
* Annotate methods, which are expected to be called with lock held, with `GuardedBy` so that error prone can analyze all invocations
### Why are the changes needed?
There is no functional change, but it helps errorprone analysis.
### 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-1079: Fix use of GuardedBy in client-flink/common [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2029:
URL: https://github.com/apache/incubator-celeborn/pull/2029#discussion_r1368997067
##########
client-flink/common/src/main/java/org/apache/celeborn/plugin/flink/buffer/TransferBufferPool.java:
##########
@@ -155,6 +155,7 @@ public void recycle(ByteBuffer buffer) {
}
}
+ @GuardedBy("lock")
Review Comment:
There is a `assert Thread.holdsLock(lock)` - so the expectation is that these two methods have the `lock` held.
This annotation is is to help error prone understand that entire method is marked thread safe w.r.t `lock`
--
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-1079: Fix use of GuardedBy in client-flink/common [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2029:
URL: https://github.com/apache/incubator-celeborn/pull/2029#issuecomment-1775620386
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2029?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#2029](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2029?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (f86d16c) into [main](https://app.codecov.io/gh/apache/incubator-celeborn/commit/49ea8810376ceb387de0ef855b7e9205101c3958?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (49ea881) will **decrease** coverage by `0.17%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## main #2029 +/- ##
==========================================
- Coverage 46.90% 46.72% -0.17%
==========================================
Files 165 165
Lines 10531 10531
Branches 959 959
==========================================
- Hits 4938 4920 -18
- Misses 5271 5285 +14
- Partials 322 326 +4
```
[see 2 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2029/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-1079: Fix use of GuardedBy in client-flink/common [incubator-celeborn]
Posted by "waitinfuture (via GitHub)" <gi...@apache.org>.
waitinfuture closed pull request #2029: CELEBORN-1079: Fix use of GuardedBy in client-flink/common
URL: https://github.com/apache/incubator-celeborn/pull/2029
--
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-1079: Fix use of GuardedBy in client-flink/common [incubator-celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2029:
URL: https://github.com/apache/incubator-celeborn/pull/2029#discussion_r1368995768
##########
client-flink/common/src/main/java/org/apache/celeborn/plugin/flink/buffer/PartitionSortedBuffer.java:
##########
@@ -92,7 +90,6 @@ public class PartitionSortedBuffer implements SortBuffer {
// For writing
// ---------------------------------------------------------------------------------------------
/** Whether this sort buffer is released. A released sort buffer can not be used. */
- @GuardedBy("lock")
Review Comment:
I am assuming this `GuardedBy` is a holdover from some past migration ?
This class is marked as not thread safe, and there is no `lock` instance.
Given this, I removed the annotation
--
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