Remove Short Circuit read/write #17243

apc999 · 2023-04-13T07:28:24Z

What changes are proposed in this pull request?

Remove source code related to Short Circuit read/write

Why are the changes needed?

In K8s env (increasingly more popular), support short-ciruit I/O is challenging
We shall use SDK instead

Does this PR introduce any user facing changes?

No more Short Circuit read/write

ssz1997 · 2023-04-19T18:18:44Z

Shall the short-circuit-related metrics be removed in this PR as well? Or metrics cleanup will be an another process?

jiacheliu3

Some hard coded tests are complaining:

2023-07-14T07:29:09.9167401Z 07:29:09.915 [ERROR] Failures: 
2023-07-14T07:29:09.9168088Z 07:29:09.915 [ERROR] alluxio.cli.fsadmin.report.MetricsCommandTest.metrics
2023-07-14T07:29:09.9169006Z 07:29:09.915 [ERROR]   Run 1: MetricsCommandTest.metrics:62->checkIfOutputValid:145 
2023-07-14T07:29:09.9175392Z Expected: iterable containing ["Cluster.BytesReadDomain  (Type: GAUGE, Value: 4145.73KB)", "Cluster.BytesReadDomainThroughput  (Type: GAUGE, Value: 29.97MB/MIN)", "Cluster.BytesReadLocal  (Type: GAUGE, Value: 11.47GB)", "Cluster.BytesReadLocalThroughput  (Type: GAUGE, Value: 117.42MB/MIN)", "Cluster.BytesReadRemote  (Type: GAUGE, Value: 401.79MB)", "Cluster.BytesReadRemoteThroughput  (Type: GAUGE, Value: 518.36MB/MIN)", "Cluster.BytesReadUfsAll  (Type: GAUGE, Value: 509.47MB)", "Cluster.BytesReadUfsThroughput  (Type: GAUGE, Value: 728.16KB/MIN)", "Cluster.BytesWrittenDomain  (Type: GAUGE, Value: 62.43MB)", "Cluster.BytesWrittenDomainThroughput  (Type: GAUGE, Value: 1202.37KB/MIN)", "Cluster.BytesWrittenRemote  (Type: GAUGE, Value: 22.98KB)", "Cluster.BytesWrittenRemoteThroughput  (Type: GAUGE, Value: 8.03MB/MIN)", "Cluster.BytesWrittenUfsAll  (Type: GAUGE, Value: 317.70KB)", "Cluster.BytesWrittenUfsThroughput  (Type: GAUGE, Value: 33.46KB/MIN)", "Cluster.CapacityTotal  (Type: GAUGE, Value: 1,154,531,246,129,122)", "Master.CompleteFileOps  (Type: COUNTER, Value: 813)", "Master.UfsSessionCount-Ufs:_alluxio_underFSStorage  (Type: COUNTER, Value: 8,535)", "Master.UfsSessionCount-Ufs:file:___Users_alluxio_alluxioMountedFolder  (Type: COUNTER, Value: 1,231)", "Master.getMetrics.User:alluxio  (Type: TIMER, Value: 4)"]
2023-07-14T07:29:09.9178398Z      but: item 2: was "Cluster.BytesReadRemote  (Type: GAUGE, Value: 401.79MB)"
2023-07-14T07:29:09.9179129Z 07:29:09.915 [ERROR]   Run 2: MetricsCommandTest.metrics:62->checkIfOutputValid:145 
2023-07-14T07:29:09.9184592Z Expected: iterable containing ["Cluster.BytesReadDomain  (Type: GAUGE, Value: 4145.73KB)", "Cluster.BytesReadDomainThroughput  (Type: GAUGE, Value: 29.97MB/MIN)", "Cluster.BytesReadLocal  (Type: GAUGE, Value: 11.47GB)", "Cluster.BytesReadLocalThroughput  (Type: GAUGE, Value: 117.42MB/MIN)", "Cluster.BytesReadRemote  (Type: GAUGE, Value: 401.79MB)", "Cluster.BytesReadRemoteThroughput  (Type: GAUGE, Value: 518.36MB/MIN)", "Cluster.BytesReadUfsAll  (Type: GAUGE, Value: 509.47MB)", "Cluster.BytesReadUfsThroughput  (Type: GAUGE, Value: 728.16KB/MIN)", "Cluster.BytesWrittenDomain  (Type: GAUGE, Value: 62.43MB)", "Cluster.BytesWrittenDomainThroughput  (Type: GAUGE, Value: 1202.37KB/MIN)", "Cluster.BytesWrittenRemote  (Type: GAUGE, Value: 22.98KB)", "Cluster.BytesWrittenRemoteThroughput  (Type: GAUGE, Value: 8.03MB/MIN)", "Cluster.BytesWrittenUfsAll  (Type: GAUGE, Value: 317.70KB)", "Cluster.BytesWrittenUfsThroughput  (Type: GAUGE, Value: 33.46KB/MIN)", "Cluster.CapacityTotal  (Type: GAUGE, Value: 1,154,531,246,129,122)", "Master.CompleteFileOps  (Type: COUNTER, Value: 813)", "Master.UfsSessionCount-Ufs:_alluxio_underFSStorage  (Type: COUNTER, Value: 8,535)", "Master.UfsSessionCount-Ufs:file:___Users_alluxio_alluxioMountedFolder  (Type: COUNTER, Value: 1,231)", "Master.getMetrics.User:alluxio  (Type: TIMER, Value: 4)"]
2023-07-14T07:29:09.9187204Z      but: item 2: was "Cluster.BytesReadRemote  (Type: GAUGE, Value: 401.79MB)"

Other than that, I browsed all changes and I think they mostly LGTM. I need to take a closer look tonight. From the scale of this PR (~2000 lines), I figure I can do a full scan of all changed lines.

If a change grows much larger than that (>3000 lines or even much larger than that), I believe it's close to impossible to do a thorough review. And for those PRs I'd recommend:

Break it into smaller reviewable parts and allow leftovers in between, as long as leftovers do not interfere other code;
Or try to allow ONLY removing code files as a whole - try to do changes as sparingly as possible because the changes cannot be reviewed or noticed at all. If a change in-line is compulsory, segregate that to one commit and log it in description. That was the approach we collectively took in Remove block interface - worker side #17638 with ~40k lines of change.

jiacheliu3

Thanks for the work!

Found some extra occurrences of the term short circuit, which are not included here. It depends on your intention of this PR whether we want to fix those individual occurrences scattered every where, or you may create a separate one to address those comment-only occurrences.

Some other relevant changes you won't be able to cover in this PR, but might need extra awareness:

In web UI there are at least some gauges for short circuit related metrics for display
Some relevant sections in the doc pages

jiacheliu3 · 2023-07-16T02:50:38Z

dora/core/common/src/main/java/alluxio/wire/MasterWebUIMetrics.java

-   *
-   * @return the cache hit local
-   */
-  public String getCacheHitLocal() {


[no action required in this PR I guess]

Seems it used to be:

local -> short circuit domain -> domain socket remote -> remote grpc not including local domain socket traffic

Now we take away "short circuit", then "local" is gone. Suddenly we only have "domain" and "remote" and no "local", although "domain" should theoretically be "local".

My point is, the opposite of "remote" should be "local" instead of "domain". Now the naming becomes a bit misleading because "local" is gone. A user may just ask "Is domain local or not?"

I'm good with the code change here, in that we are just removing code instead of doing a refactor. It's just we may need to rename the metrics in the next refactor attempt.

agree. let's revisit the metrics name later. I will leave a TODO here

jiacheliu3 · 2023-07-16T03:01:24Z

dora/core/client/fs/src/main/java/alluxio/client/block/stream/BlockInStream.java

@@ -115,49 +114,27 @@ public static BlockInStream create(FileSystemContext context, BlockInfo info,
    }

    AlluxioConfiguration alluxioConf = context.getClusterConf();
-    boolean shortCircuit = alluxioConf.getBoolean(PropertyKey.USER_SHORT_CIRCUIT_ENABLED);


In the comment of this method, there's "short-circuit" mentioned.

jiacheliu3 · 2023-07-16T03:02:31Z

dora/core/client/fs/src/main/java/alluxio/client/block/stream/DataWriter.java

@@ -57,9 +57,6 @@ private Factory() {} // prevent instantiation
    public static DataWriter create(FileSystemContext context, long blockId, long blockSize,
        WorkerNetAddress address, OutStreamOptions options) throws IOException {
      AlluxioConfiguration alluxioConf = context.getClusterConf();
-      boolean shortCircuit = alluxioConf.getBoolean(PropertyKey.USER_SHORT_CIRCUIT_ENABLED);


In GrpcDataWriter there are comments mentioning "short-circuit". I found those occurrences by searching "shortcircuit" and "short-circuit" and "short circuit".

Some other occurrences include Format.java, package-info.java, BlockInStream.java, DecommissionWorkerCommand etc.

all removed

apc999

Java comment part addressed. PTAL

For webui/metrics, we shall have a separate round of revamp

apc999 · 2023-07-16T06:52:17Z

dora/core/client/fs/src/main/java/alluxio/client/block/stream/BlockInStream.java

@@ -115,49 +114,27 @@ public static BlockInStream create(FileSystemContext context, BlockInfo info,
    }

    AlluxioConfiguration alluxioConf = context.getClusterConf();
-    boolean shortCircuit = alluxioConf.getBoolean(PropertyKey.USER_SHORT_CIRCUIT_ENABLED);


apc999 · 2023-07-16T07:20:51Z

dora/core/client/fs/src/main/java/alluxio/client/block/stream/DataWriter.java

@@ -57,9 +57,6 @@ private Factory() {} // prevent instantiation
    public static DataWriter create(FileSystemContext context, long blockId, long blockSize,
        WorkerNetAddress address, OutStreamOptions options) throws IOException {
      AlluxioConfiguration alluxioConf = context.getClusterConf();
-      boolean shortCircuit = alluxioConf.getBoolean(PropertyKey.USER_SHORT_CIRCUIT_ENABLED);


all removed

jiacheliu3

LGTM, thanks for the work!

apc999 · 2023-07-16T15:50:12Z

alluxio-bot, merge this please

alluxio-bot · 2023-07-16T15:50:15Z

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

apc999 · 2023-07-16T15:50:39Z

alluxio-bot, merge this please

alluxio-bot added the API Change Changes covering public API label Apr 13, 2023

alluxio-bot assigned tcrain Apr 13, 2023

apc999 force-pushed the dora-remove-shortcircuit branch from 8b14b24 to 977fde5 Compare April 18, 2023 05:25

apc999 requested a review from beinan April 18, 2023 05:25

apc999 force-pushed the dora-remove-shortcircuit branch 3 times, most recently from 8605ed6 to dc05cb1 Compare April 19, 2023 17:44

Xenorith force-pushed the dora branch 3 times, most recently from 253ed31 to 6ecdda5 Compare April 28, 2023 21:46

Remove ShortCircuit read/write

8252255

apc999 force-pushed the dora-remove-shortcircuit branch from dc05cb1 to 8252255 Compare July 14, 2023 06:28

apc999 requested review from jiacheliu3 and removed request for beinan July 14, 2023 06:49

Remove metrics

696d66a

jiacheliu3 reviewed Jul 14, 2023

View reviewed changes

jiacheliu3 requested changes Jul 16, 2023

View reviewed changes

More fix

a258d3e

apc999 commented Jul 16, 2023

View reviewed changes

jiacheliu3 approved these changes Jul 16, 2023

View reviewed changes

apc999 added the type-debt This issue is about tech debt label Jul 16, 2023

alluxio-bot merged commit 7f98cab into Alluxio:main Jul 16, 2023
15 checks passed

apc999 deleted the dora-remove-shortcircuit branch July 18, 2023 04:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Short Circuit read/write #17243

Remove Short Circuit read/write #17243

apc999 commented Apr 13, 2023

ssz1997 commented Apr 19, 2023

jiacheliu3 left a comment

jiacheliu3 left a comment

jiacheliu3 Jul 16, 2023

jiacheliu3 Jul 16, 2023

apc999 Jul 16, 2023

jiacheliu3 Jul 16, 2023

apc999 Jul 16, 2023

jiacheliu3 Jul 16, 2023

jiacheliu3 Jul 16, 2023

apc999 Jul 16, 2023

apc999 left a comment

apc999 Jul 16, 2023

apc999 Jul 16, 2023

jiacheliu3 left a comment

apc999 commented Jul 16, 2023

alluxio-bot commented Jul 16, 2023

apc999 commented Jul 16, 2023

Remove Short Circuit read/write #17243

Remove Short Circuit read/write #17243

Conversation

apc999 commented Apr 13, 2023

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

ssz1997 commented Apr 19, 2023

jiacheliu3 left a comment

Choose a reason for hiding this comment

jiacheliu3 left a 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

apc999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiacheliu3 left a comment

Choose a reason for hiding this comment

apc999 commented Jul 16, 2023

alluxio-bot commented Jul 16, 2023

apc999 commented Jul 16, 2023