-
Notifications
You must be signed in to change notification settings - Fork 140
[MRESOLVER-153] Make TrackingFileManager use NamedLocks #101
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
Conversation
Step1: drop all SyncContextFactory implementations and indirections, redo the same functionality using NamedLocks.
Sounds more correct.
It was in SISU (due SISU index) and in ServiceLocator (kinda), but was not in Guice, but it worked, as it has default ctor. Once default ctor (along with ServiceLocator) is gone, these errors will be easier to spot.
...resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java
Outdated
Show resolved
Hide resolved
...resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java
Outdated
Show resolved
Hide resolved
|
||
RandomAccessFile raf = null; | ||
try | ||
try ( NamedLock lock = namedLockFactory.getLock( getFileKey( file ) ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suffers now from one problem. Since we don't use name mappers you will have the SAME lock name on two different CI nodes with Redisson using the same directory structure. They will block each other for no reason. Makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use selector here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on two different nodes using two different storage mounts (disks), why'd you use redisson to sync those two (unrelated) nodes at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that I expect consistent approach to name mapping wherever the lock factory is used. I need to think about this more tomorrow.
...resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java
Outdated
Show resolved
Hide resolved
...mpl/src/main/java/org/eclipse/aether/internal/impl/synccontext/NamedLockFactorySelector.java
Outdated
Show resolved
Hide resolved
...resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
@cstamas note to myself: split this in two: drop SyncContextFactory imples (other than named), and implement locking in TFM |
Resolve #925 |
1 similar comment
Resolve #925 |
Now that TrackingFileManager is shared singleton, let's make it use NamedLockFactory.
Commits:
Step1: drop all SyncContextFactory implementations and indirections, redo the same functionality using NamedLocks.
Step2: Pull out NamedLockFactory selection and expose selected one.
Step3: Tie up selected NamedLockFactory to TrackingFileManager.
Step4: use locking
Step5 keying for files., prefix + sha1(path)