Update worker api support for load multi replicas #18296

jja725 · 2023-10-18T21:08:14Z

What changes are proposed in this pull request?

Update worker api support for load multi replicas

Why are the changes needed?

part of PR to support load multi replicas

Does this PR introduce any user facing changes?

na

alluxio-bot · 2023-10-18T23:21:28Z

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 must be capitalized
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.

alluxio-bot · 2023-10-18T23:38:29Z

Automated checks report:

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

All checks passed!

lucyge2022 · 2023-10-23T22:12:53Z

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

+            }
+            long fileLength = block.getUfsStatus().getUfsFileStatus().getContentLength();
+            if (block.hasMainWorker()) {
+              WorkerNetAddress address = GrpcUtils.fromProto(block.getMainWorker());


main worker is set from scheduler? if we r submitting load tasks to all replicas at same time, how to ensure the main worker would have loaded at the time of secondary worker trying to read from it?

Right now we don't ensure that due to the concurrency issue of getAndLoad. So we would recommend to user to load one replica first and then set multiple replicas to ensure they only read the data from ufs once. If we want to further improve this part we can improve the getAndLoad in CacheManager.

understood, fix getAndLoad to save only one read is out of scope on this PR, this PR targets at only save duplicate read from multiple workers not duplicate read from one worker.

lucyge2022 · 2023-10-23T22:38:51Z

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

+  @VisibleForTesting
+  public void loadDataFromRemote(String filePath, long offset, long lengthToLoad,
+      PositionReader reader, int chunkSize) throws IOException {
+    ByteBuffer buf = ByteBuffer.allocate(chunkSize);


use PooledDirectNioByteBuf.allocate(chunkSize) as loadData does since most of it will be aligned to pagesize, and pass in NettyBufTargetBuffer type for reader.read(long position, ReadTargetBuffer buffer, int length) as this pool can manage a reuse of these mostly aligned buffer. otherwise this is allocate onheap will cause huge mem footprint and put heavylifting on GC

oh u use ByteBuffer bcos of cachemanager only support bytebuffer, then its better to allocate direct buffer than heap, large buffer mem footprint on heap might cause program misbehavior.

use direct buffer

lucyge2022 · 2023-10-23T22:56:11Z

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

+    ByteBuffer buf = ByteBuffer.allocate(chunkSize);
+    String fileId = new AlluxioURI(filePath).hash();
+
+    while (0 < lengthToLoad) {


lengthToLoad > 0 ?

lucyge2022 · 2023-10-23T23:01:13Z

common/transport/src/main/proto/grpc/file_system_master.proto

@@ -608,6 +608,7 @@ message LoadJobPOptions {
  optional bool partialListing = 3;
  optional bool loadMetadataOnly = 4;
  optional bool skipIfExists = 5;
+  optional int32 replicas = 6;


im assuming this is not used in this PR right?

would be used in following PR

dbw9580 · 2023-10-24T15:02:40Z

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

+                try (PositionReader reader = new NettyDataReader(mFsContext, address, builder)) {
+                  loadDataFromRemote(block.getUfsPath(), block.getOffsetInFile(), block.getLength(),


Now this is worker-to-worker communication, but reusing client-side code. This can cause some metrics to go inaccurate as the worker being requested cannot know whether the peer is really a client or another worker.

Can we have a different request type other than Protocol.ReadRequest so that the other worker can tell whether this is a normal read request from a true client, or from a peer worker for caching purposes? This allows splitting the code paths and reduce intertwined code.

The NettyDataReader should be moved into the common module, also it should be able to handle a generic *Request that involves data transmission.

I would propose to add a field user in the ReadRequest to indicate who is issuing the read so we can distinguish the reader(client or worker) cause iterally worker is just another user to send the read request. And I would try to do the refactoring of NettyDataReader in a later PR so we can have limited scope in this PR

dbw9580 · 2023-10-24T15:14:40Z

common/transport/src/main/proto/grpc/block_worker.proto

@@ -226,6 +226,7 @@ message Block{
  optional int64 offset_in_file = 4;
  optional int64 mountId = 5;
  optional UfsStatus ufs_status = 6;
+  optional WorkerNetAddress main_worker = 7;


How is the main worker of a block defined? Is it a consistent relationship between a block and a main worker? What happens when the main worker is not available?

If the client wants to express the idea that "for this load job and this particular block, load from this worker," then I don't think we need to involve the concept of a main worker. Instead, you can define the LoadRequest object as

message LoadRequest { message BlockLoadRequest { optional Block blockToLoad = 1; optional WorkerNetAddress workerToLoadFrom = 2; } repeated BlockLoadRequest blocks = 1; // ... other fields }

use a new proto message loaddatasubtask to reduce confusion.

lucyge2022 · 2023-10-24T21:30:07Z

common/transport/src/main/proto/grpc/block_worker.proto

-  optional Block block = 1;
-  optional UfsStatus ufs_status = 2;
+  optional LoadDataSubTask load_data_subtask = 1;
+  optional UfsStatus load_metadata_subtask = 2;


in that sense its better to make a LoadMetadataSubTask in future refactor? but no need to address in current PR

done, avoid future incompatibility

lucyge2022

LGTM

dbw9580

LGTM

jja725 · 2023-10-26T02:10:15Z

alluxio-bot, merge this please

alluxio-bot · 2023-10-26T02:10:17Z

merge failed:
Merge refused because pull request does not have label start with type-

jja725 · 2023-10-26T02:10:29Z

alluxio-bot, merge this please

### What changes are proposed in this pull request? Update worker api support for load multi replicas ### Why are the changes needed? part of PR to support load multi replicas ### Does this PR introduce any user facing changes? na pr-link: Alluxio#18296 change-id: cid-0213f2aba669b7687ac42cf932cdcec911d397a4

jja725 force-pushed the load-replis branch from 7693bf7 to da32565 Compare October 18, 2023 23:21

jja725 force-pushed the load-replis branch from da32565 to 4df1a37 Compare October 18, 2023 23:36

jja725 requested review from lucyge2022 and dbw9580 October 18, 2023 23:37

jja725 changed the title ~~add support for load multi replicas~~ add worker api support for load multi replicas Oct 18, 2023

jja725 changed the title ~~add worker api support for load multi replicas~~ Update worker api support for load multi replicas Oct 18, 2023

lucyge2022 reviewed Oct 23, 2023

View reviewed changes

dbw9580 suggested changes Oct 24, 2023

View reviewed changes

jja725 added 2 commits October 24, 2023 13:53

add support for load multi replicas

583e5de

address comment

b945db7

jja725 force-pushed the load-replis branch 2 times, most recently from 7658a84 to a0bfcac Compare October 24, 2023 21:05

lucyge2022 reviewed Oct 24, 2023

View reviewed changes

lucyge2022 approved these changes Oct 24, 2023

View reviewed changes

jja725 force-pushed the load-replis branch from a0bfcac to c5e589b Compare October 24, 2023 21:38

rename proto to make semantic clear

9ea74cc

jja725 force-pushed the load-replis branch from c5e589b to 9ea74cc Compare October 24, 2023 21:48

dbw9580 approved these changes Oct 26, 2023

View reviewed changes

jja725 added the type-feature This issue is a feature request label Oct 26, 2023

alluxio-bot merged commit 7a5734f into Alluxio:main Oct 26, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update worker api support for load multi replicas #18296

Update worker api support for load multi replicas #18296

jja725 commented Oct 18, 2023 •

edited

Loading

alluxio-bot commented Oct 18, 2023

alluxio-bot commented Oct 18, 2023

lucyge2022 Oct 23, 2023

jja725 Oct 23, 2023 •

edited

Loading

lucyge2022 Oct 23, 2023 •

edited

Loading

lucyge2022 Oct 23, 2023

lucyge2022 Oct 23, 2023

jja725 Oct 23, 2023

lucyge2022 Oct 23, 2023

jja725 Oct 23, 2023

lucyge2022 Oct 23, 2023

jja725 Oct 23, 2023

dbw9580 Oct 24, 2023

dbw9580 Oct 24, 2023

jja725 Oct 24, 2023 •

edited

Loading

dbw9580 Oct 24, 2023

jja725 Oct 24, 2023

lucyge2022 Oct 24, 2023

jja725 Oct 24, 2023

lucyge2022 left a comment

dbw9580 left a comment

jja725 commented Oct 26, 2023

alluxio-bot commented Oct 26, 2023

jja725 commented Oct 26, 2023

		try (PositionReader reader = new NettyDataReader(mFsContext, address, builder)) {
		loadDataFromRemote(block.getUfsPath(), block.getOffsetInFile(), block.getLength(),

Update worker api support for load multi replicas #18296

Update worker api support for load multi replicas #18296

Conversation

jja725 commented Oct 18, 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 Oct 18, 2023

alluxio-bot commented Oct 18, 2023

Choose a reason for hiding this comment

jja725 Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

lucyge2022 Oct 23, 2023 • edited Loading

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

jja725 Oct 24, 2023 • edited Loading

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

lucyge2022 left a comment

Choose a reason for hiding this comment

dbw9580 left a comment

Choose a reason for hiding this comment

jja725 commented Oct 26, 2023

alluxio-bot commented Oct 26, 2023

jja725 commented Oct 26, 2023

jja725 commented Oct 18, 2023 •

edited

Loading

jja725 Oct 23, 2023 •

edited

Loading

lucyge2022 Oct 23, 2023 •

edited

Loading

jja725 Oct 24, 2023 •

edited

Loading