benwtrent commented on code in PR #13142:
URL: https://github.com/apache/lucene/pull/13142#discussion_r1524632573
##########
lucene/core/src/java/org/apache/lucene/store/TrackingDirectoryWrapper.java:
##########
@@ -61,10 +61,8 @@ public void copyFrom(Directory from, String src, String
dest, IOContext context)
@Override
public void rename(String source, String dest) throws IOException {
in.rename(source, dest);
- synchronized (createdFileNames) {
- createdFileNames.add(dest);
- createdFileNames.remove(source);
- }
+ createdFileNames.add(dest);
+ createdFileNames.remove(source);
Review Comment:
This means that another thread can mutate `createdFileNames` between `add`
and `remove`. Are we sure this doesn't add any weird race conditions?
##########
lucene/replicator/src/java/org/apache/lucene/replicator/nrt/ReplicaNode.java:
##########
@@ -625,19 +624,17 @@ public void run(CopyJob job) {
for (String fileName : curNRTCopy.getFileNamesToCopy()) {
assert lastCommitFiles.contains(fileName) == false
: "fileName=" + fileName + " is in lastCommitFiles and is being
copied?";
- synchronized (mergeCopyJobs) {
- for (CopyJob mergeJob : mergeCopyJobs) {
- if (mergeJob.getFileNames().contains(fileName)) {
- // TODO: we could maybe transferAndCancel here? except CopyJob
can't transferAndCancel
- // more than one currently
- message(
- "top: now cancel merge copy job="
- + mergeJob
- + ": file "
- + fileName
- + " is now being copied via NRT point");
- mergeJob.cancel("newNRTPoint is copying over the same file", null);
- }
+ for (CopyJob mergeJob : mergeCopyJobs) {
+ if (mergeJob.getFileNames().contains(fileName)) {
Review Comment:
Since this no longer locks on `mergeCopyJobs`, this means during the
iteration, the collection can be mutated. Are we sure this is OK?
##########
lucene/replicator/src/java/org/apache/lucene/replicator/nrt/ReplicaNode.java:
##########
@@ -625,19 +624,17 @@ public void run(CopyJob job) {
for (String fileName : curNRTCopy.getFileNamesToCopy()) {
assert lastCommitFiles.contains(fileName) == false
: "fileName=" + fileName + " is in lastCommitFiles and is being
copied?";
- synchronized (mergeCopyJobs) {
- for (CopyJob mergeJob : mergeCopyJobs) {
- if (mergeJob.getFileNames().contains(fileName)) {
- // TODO: we could maybe transferAndCancel here? except CopyJob
can't transferAndCancel
- // more than one currently
- message(
- "top: now cancel merge copy job="
- + mergeJob
- + ": file "
- + fileName
- + " is now being copied via NRT point");
- mergeJob.cancel("newNRTPoint is copying over the same file", null);
- }
+ for (CopyJob mergeJob : mergeCopyJobs) {
+ if (mergeJob.getFileNames().contains(fileName)) {
Review Comment:
`mergeCopyJobs` is protected, implying its use with sub-classes. You can see
this in `SimpleReplicaNode`, which takes action while
`synchronized(mergeCopyJobs)`.
A quick search on github shows that other repositories take similar action.
Are we sure this doesn't add a race condition to library user's code that
sub-class the `ReplicaNode` class & synchronize on `mergeCopyJobs`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
