Refactor PagedDoraWorker by injecting MetaManager and UfsManager #18181

jiacheliu3 · 2023-09-20T13:13:05Z

What changes are proposed in this pull request?

If an object is created inside PagedDoraWorker constructor, extract that creation to before the constructor and use dependency injection to inject it to the worker object. This doesn't change any creation logic, just a refactor to better adapt to dependency injection flavor.
There is a circular dependency between MetaManager and PagedDoraWorker. This change removes that cycle. Now we create one, then create the other. Before, we create one and in the construction, we let this ref escape and create the other. Some methods are either moved or changed to static.
By adapting to dependency injection, we rely on UfsManager interface instead of DoraUfsManager implementation. Some method signatures are extracted to the interface level.
A few other small refactors to get rid of some downcasts and variable scope changes. Reasons are attached in comments on this PR.

Why are the changes needed?

Improve code quality and extensibility.

Does this PR introduce any user facing changes?

No. All refactor changes are small and equivalent to existing code. So nothing should break.

alluxio-bot · 2023-09-20T14:18:55Z

Automated checks report:

PR title follows the conventions: FAIL
- The title of the PR does not pass all the checks. Please fix the following issues:
  - First word of title ("Dep") is not an imperative verb. Please use one of the valid words
Commits associated with Github account: PASS

Some checks failed. Please fix the reported issues and reply 'alluxio-bot, check this please' to re-run checks.

jiacheliu3 · 2023-10-30T04:12:38Z

dora/core/server/worker/src/test/java/alluxio/worker/dora/DoraMetaManagerTest.java

@@ -57,7 +57,7 @@ public void before() throws IOException {
    PagedDoraWorker worker = mock(PagedDoraWorker.class);
    CacheManager cacheManager = mock(CacheManager.class);
    mDoraUfsManager = mock(DoraUfsManager.class);
-    mManager = new DoraMetaManager(conf, worker, cacheManager, mDoraUfsManager);
+    mManager = new DoraMetaManager(conf, cacheManager, mDoraUfsManager);


this line is one of the core change in this PR. The PagedDoraWorker no longer passes its this out in its own constructor. The this ref escaping in the constructure is a bad practice we should avoid.

jiacheliu3 · 2023-10-30T04:13:53Z

dora/core/server/worker/src/main/java/alluxio/worker/dora/PagedDoraWorker.java

    super(ExecutorServiceFactories.fixedThreadPool("dora-worker-executor", 5));
    mWorkerId = workerId;
    mConf = conf;
-    mUfsManager = mResourceCloser.register(new DoraUfsManager());
+    mUfsManager = mResourceCloser.register(ufsManager);


I'm just creating the UfsManager outside this constructor and passing it in. The code logic does not change at all so this refactor should be effectively minor.

Guice advises against injecting closeable resources especially when it's a singleton, as it's hard to reason about who is responsible to close the resource. https://github.com/google/guice/wiki/Avoid-Injecting-Closable-Resources

Yea for best coding style we probably should refactor this a bit (together with our next big modularization efforts).
For the current code, PagedDoraWorker.close() is the end of worker process and I figure it's fine we let the closer close that singleton.

jiacheliu3 · 2023-10-30T04:15:11Z

dora/core/server/worker/src/main/java/alluxio/worker/dora/PagedDoraWorker.java

@@ -209,8 +201,7 @@ protected PagedDoraWorker(
    mPageSize = mConf.getBytes(PropertyKey.WORKER_PAGE_STORE_PAGE_SIZE);
    mBlockMasterClientPool = blockMasterClientPool;
    mCacheManager = cacheManager;
-    mMetaManager = mResourceCloser.register(
-        new DoraMetaManager(mConf, this, mCacheManager, mUfsManager));
+    mMetaManager = mResourceCloser.register(metaManager);


Interestingly, I don't have something like bind(MetaManager).to(DoraMetaManager.class) in DoraWorkerModule and Guice is actually able to notice that and create the MetaManager instance for me. Is that okay? I figure we should always call bind() explicitly in code?

DoraMetaManager is not an interface but a concrete class. There is no MetaManager.

jiacheliu3 · 2023-10-30T04:15:51Z

dora/core/server/worker/src/main/java/alluxio/worker/dora/PagedDoraWorker.java

-   * @param xattrMap
-   * @return a FileInfo
-   */
-  public alluxio.grpc.FileInfo buildFileInfoFromUfsStatus(UfsStatus status, String ufsFullPath,


These two methods are just changed static and moved to the end of this file.

jiacheliu3 · 2023-10-30T04:17:28Z

dora/core/server/worker/src/main/java/alluxio/worker/modules/DoraWorkerModule.java

+    // Create FileSystemContext shared across all worker components
+    FileSystemContext fileSystemContext = FileSystemContext.create();
+    bind(FileSystemContext.class).toInstance(fileSystemContext);


Similar to the change in PagedDoraWorker constructor, I'm just moving some resource creation out of PagedDoraWorker() into this module definition, and injecting into the constructor. The code logic effectively doesn't change.

dora/core/server/worker/src/main/java/alluxio/worker/dora/PagedDoraWorker.java

jiacheliu3 · 2023-10-30T04:30:47Z

dora/core/server/worker/src/main/java/alluxio/worker/DataServerFactory.java

      blockWorkerService =
-          new DoraWorkerClientServiceHandler((DoraWorker) dataWorker);
+          new DoraWorkerClientServiceHandler((PagedDoraWorker) dataWorker);


DoraWorker interface is missing a lot of method definitions so we need to downcast in DoraWorkerClientServiceHandler after all. So I'm just casting to the real class to avoid the frequent casting.

jiacheliu3 · 2023-10-30T06:50:04Z

dora/core/server/worker/src/main/java/alluxio/worker/dora/PagedDoraWorker.java

  // TODO(lucy) change to string typed once membership manager got enabled by default
  private final AtomicReference<WorkerIdentity> mWorkerId;
  protected final CacheManager mCacheManager;
-  protected final DoraUfsManager mUfsManager;
-  private final DoraMetaManager mMetaManager;
+  protected final UfsManager mUfsManager;


Use interface here since we are using dependency injection. If one method is not defined on the interface (in the concrete class), I'm moving the signature to the interface level.

jiacheliu3 · 2023-10-30T06:56:18Z

dora/core/server/worker/src/main/java/alluxio/worker/dora/PagedDoraWorker.java

-  protected final DoraUfsManager mUfsManager;
-  private final DoraMetaManager mMetaManager;
+  protected final UfsManager mUfsManager;
+  protected DoraMetaManager mMetaManager;


it's necessary to change the scope here so a child class can see this field

jiacheliu3 · 2023-10-30T06:56:52Z

dora/core/server/worker/src/main/java/alluxio/worker/dora/PagedDoraWorker.java

+   * @param xattrMap
+   * @return a FileInfo
+   */
+  public static alluxio.grpc.FileInfo buildFileInfoFromUfsStatus(


there's probably a better place for these 2 static methods but I don't see it right now

alluxio-bot · 2023-10-30T14:07:21Z

Automated checks report:

PR title follows the conventions: PASS
Commits associated with Github account: PASS

All checks passed!

dbw9580

LGTM

jiacheliu3 · 2023-11-01T14:06:13Z

alluxio-bot, merge this please

jiacheliu3 · 2023-11-06T13:38:03Z

alluxio-bot, cherry-pick this to branch-3-1.1.x please

alluxio-bot · 2023-11-06T13:38:31Z

Auto cherry-pick to branch branch-3-1.1.x successfully opened PR: #18381

### What changes are proposed in this pull request? 1. If an object is created inside `PagedDoraWorker` constructor, extract that creation to before the constructor and use dependency injection to inject it to the worker object. This doesn't change any creation logic, just a refactor to better adapt to dependency injection flavor. 2. There is a circular dependency between `MetaManager` and `PagedDoraWorker`. This change removes that cycle. Now we create one, then create the other. Before, we create one and in the construction, we let `this` ref escape and create the other. Some methods are either moved or changed to `static`. 3. By adapting to dependency injection, we rely on `UfsManager` interface instead of `DoraUfsManager` implementation. Some method signatures are extracted to the interface level. 4. A few other small refactors to get rid of some downcasts and variable scope changes. Reasons are attached in comments on this PR. ### Why are the changes needed? Improve code quality and extensibility. ### Does this PR introduce any user facing changes? No. All refactor changes are small and equivalent to existing code. So nothing should break. pr-link: #18181 change-id: cid-4f9e9bc770b12253188bb541dd456ef3cd889c2b

Cherry-pick of existing commit. orig-pr: #18181 orig-commit: 0e83207 orig-commit-author: Jiacheng Liu <[email protected]> pr-link: #18381 change-id: cid-4f9e9bc770b12253188bb541dd456ef3cd889c2b

### What changes are proposed in this pull request? 1. If an object is created inside `PagedDoraWorker` constructor, extract that creation to before the constructor and use dependency injection to inject it to the worker object. This doesn't change any creation logic, just a refactor to better adapt to dependency injection flavor. 2. There is a circular dependency between `MetaManager` and `PagedDoraWorker`. This change removes that cycle. Now we create one, then create the other. Before, we create one and in the construction, we let `this` ref escape and create the other. Some methods are either moved or changed to `static`. 3. By adapting to dependency injection, we rely on `UfsManager` interface instead of `DoraUfsManager` implementation. Some method signatures are extracted to the interface level. 4. A few other small refactors to get rid of some downcasts and variable scope changes. Reasons are attached in comments on this PR. ### Why are the changes needed? Improve code quality and extensibility. ### Does this PR introduce any user facing changes? No. All refactor changes are small and equivalent to existing code. So nothing should break. pr-link: Alluxio#18181 change-id: cid-4f9e9bc770b12253188bb541dd456ef3cd889c2b

apc999 force-pushed the main branch 2 times, most recently from 2fec0ec to b597c61 Compare October 17, 2023 19:11

jiacheliu3 added 4 commits October 25, 2023 17:12

remove properties from key

6b6bdb8

other changes to make dep injection happen

a78c235

fix incorrect method calls

e604f92

fix leftovers

28ad411

jiacheliu3 force-pushed the dep-injection branch from 4ff13c3 to 28ad411 Compare October 25, 2023 15:33

better format

2cc9b50

jiacheliu3 commented Oct 30, 2023

View reviewed changes

dora/core/server/worker/src/main/java/alluxio/worker/dora/PagedDoraWorker.java Show resolved Hide resolved

jiacheliu3 added 2 commits October 30, 2023 12:22

move static to end of file

53caade

Merge remote-tracking branch 'upstream/main' into dep-injection

555e55a

jiacheliu3 commented Oct 30, 2023

View reviewed changes

jiacheliu3 added 2 commits October 30, 2023 12:41

member variable type using interface

e377e92

make it compile

6b49e8a

jiacheliu3 commented Oct 30, 2023

View reviewed changes

small improvements

e334d2b

jiacheliu3 changed the title ~~Dep injection~~ Inject MetaManager and UfsManager to PagedDoraWorker Oct 30, 2023

jiacheliu3 changed the title ~~Inject MetaManager and UfsManager to PagedDoraWorker~~ Refactor PagedDoraWorker by injecting MetaManager and UfsManager Oct 30, 2023

jiacheliu3 requested review from dbw9580, beinan and JiamingMai October 30, 2023 14:12

dbw9580 approved these changes Oct 31, 2023

View reviewed changes

jiacheliu3 added 3 commits November 1, 2023 16:57

checkstyle

26518b5

explicitly bind DoraMetaManager

e459a1e

avoid binding to itself

d2259bd

jiacheliu3 added the type-code-quality code quality improvement label Nov 1, 2023

alluxio-bot merged commit 0e83207 into main Nov 1, 2023
12 checks passed

alluxio-bot mentioned this pull request Nov 6, 2023

Refactor PagedDoraWorker by injectin...; Port [#18181] to branch-3-1.1.x #18381

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor PagedDoraWorker by injecting MetaManager and UfsManager #18181

Refactor PagedDoraWorker by injecting MetaManager and UfsManager #18181

jiacheliu3 commented Sep 20, 2023 •

edited

Loading

alluxio-bot commented Sep 20, 2023

jiacheliu3 Oct 30, 2023

jiacheliu3 Oct 30, 2023

dbw9580 Oct 31, 2023

jiacheliu3 Nov 1, 2023

jiacheliu3 Oct 30, 2023

dbw9580 Oct 31, 2023

jiacheliu3 Oct 30, 2023

jiacheliu3 Oct 30, 2023

jiacheliu3 Oct 30, 2023

jiacheliu3 Oct 30, 2023

jiacheliu3 Oct 30, 2023

jiacheliu3 Oct 30, 2023

alluxio-bot commented Oct 30, 2023

dbw9580 left a comment

jiacheliu3 commented Nov 1, 2023

jiacheliu3 commented Nov 6, 2023

alluxio-bot commented Nov 6, 2023

Refactor PagedDoraWorker by injecting MetaManager and UfsManager #18181

Refactor PagedDoraWorker by injecting MetaManager and UfsManager #18181

Conversation

jiacheliu3 commented Sep 20, 2023 • edited Loading

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

alluxio-bot commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alluxio-bot commented Oct 30, 2023

dbw9580 left a comment

Choose a reason for hiding this comment

jiacheliu3 commented Nov 1, 2023

jiacheliu3 commented Nov 6, 2023

alluxio-bot commented Nov 6, 2023

jiacheliu3 commented Sep 20, 2023 •

edited

Loading