Adding parallelism to the ETH, ERC20, MATIC pipelines #162

spiderjako · 2023-11-18T13:16:36Z

No description provided.

YordanPavlov · 2023-11-28T07:34:35Z

blockchains/erc20/erc20_worker.js

-    const result = constants.EXPORT_BLOCKS_LIST ?
-      this.getBlocksListInterval() :
-      await nextIntervalCalculator(this);
+    const requestIntervals = await nextIntervalCalculator(this);


Are you removing the 'export block list' feature?

I've completely missed that. But for the time being, I'd remove this part of the code and rework it later on. It needs to be reworked to fit in with this functionality. WDYT?

I agree, let's integrate it again in a next PR not to slow down this one. It would be very similar to what you do in nextIntervalCalculator currently - generate a sequence of intervals with the parallelism in mind.

YordanPavlov · 2023-11-28T08:08:42Z

blockchains/eth/eth_worker.js

+    let retryIntervalMs = 0;
+    while (retries < constants.MAX_RETRIES) {
+      try {
+        const response = await this.ethClient.request(...params);


should (...params) be spread again, I thought that request would work with an array

I think that it would work with an array if it works with a batch of requests. Technically, [] is a batch, but I'd have to rework how the parameters are given. Currently we have

('trace_filter', [{ fromBlock: this.web3Wrapper.parseNumberToHex(fromBlock), toBlock: this.web3Wrapper.parseNumberToHex(toBlock) }])

YordanPavlov · 2023-11-28T08:14:09Z

blockchains/eth/eth_worker.js

+          retryIntervalMs += (2000 * retries);
+          logger.error(`${params[0]} failed. Reason: ${response.error}. Retrying for ${retries} time`);
+          await new Promise((resolve) => setTimeout(resolve, retryIntervalMs));
+          continue;


Wouldn't if-else, be cleaner than continue?

YordanPavlov · 2023-11-28T08:14:23Z

blockchains/eth/eth_worker.js

+        const response = await this.ethClient.request(...params);
+        if (response.error || response.result === null) {
+          retries++;
+          retryIntervalMs += (2000 * retries);


Use const value for 2000, like constants.BACKOFF_RETRY_STEP

blockchains/eth/eth_worker.js

YordanPavlov · 2023-11-28T08:19:35Z

blockchains/eth/eth_worker.js

+      try {
+        const response = await this.ethClient.request(...params);
+        if (response.error || response.result === null) {
+          retries++;


Should we even wait if the retries number is reached at this point, think MAX_RETRIES=0

we should definitely wait. I suggest to use a library for retryer. it's error prone to write it yourself

What do you mean?

I would expect MAX_RETRIES to specify the number of retries, that is attempts to re-send after the initial sending. Instead if it is is 0, we would not send the request at all. If it is 1, we would send one time, on error wait, but then not retry. Seems to me like the logic is a bit off.

YordanPavlov · 2023-11-28T08:19:44Z

blockchains/eth/eth_worker.js

+        retries++;
+        retryIntervalMs += (2000 * retries);
+        logger.error(
+          `Try block in ${params[0]} failed. Reason: ${err.toString()}. Waiting ${retryIntervalMs} and retrying for ${retries} time`
+          );
+        await new Promise((resolve) => setTimeout(resolve, retryIntervalMs));


This logic seems to be the same as above, can it be moved to waitOnError() method?

I'd leave it as it is and rework both this and the UTXO version in another PR. They currently have basically the same idea. Maybe I'll even combine them. Currently, the loggings point to different errors in general, one is to the response's error, the other is a caught error

YordanPavlov · 2023-11-28T08:40:09Z

blockchains/eth/lib/next_interval_calculator.js

+  worker.sleepTimeMsec = 0;
+  const progressDifference = worker.lastConfirmedBlock - worker.lastExportedBlock;
+  const maxInterval = constants.MAX_CONCURRENT_REQUESTS * constants.BLOCK_INTERVAL;
+  let intervalArrayLength;


Can be made const with the ternary operator.

Could be, but IMO it looks cleaner this way. With ternary it would be a bit too bloated. I like to flex with ternary but I realised a few weeks back that I don't have to always do it. I don't think it's a big deal that it isn't a const, as it's basically given as an argument at the next line and the function ends.

It's not about flexing, const variables make it easier to reason about the code. Minor ofc, your call.

blockchains/eth/lib/next_interval_calculator.js

WonderBeat

We implemented fork-join for a streaming task. this works, but doesn't utilize fully our bottleneck - bc node. Each iteration we spawn N concurrent requests and wait for all of them. We wait till the slowest request in a batch completes. It may be retried several times before completion. still we will wait for it.
Sorry, I believe that this is not the best design for the data transferring app.

WonderBeat · 2023-11-28T10:35:38Z

blockchains/eth/eth_worker.js

+      try {
+        const response = await this.ethClient.request(...params);
+        if (response.error || response.result === null) {
+          retries++;


we should definitely wait. I suggest to use a library for retryer. it's error prone to write it yourself

WonderBeat · 2023-11-28T10:41:05Z

blockchains/eth/eth_worker.js

+          await new Promise((resolve) => setTimeout(resolve, retryIntervalMs));
+          continue;
+        }
+        return response;


Am I right that this code path returns data structure and the path with a failure returns Promise? This looks odd

YordanPavlov · 2023-11-28T12:51:11Z

blockchains/erc20/erc20_worker.js

-    const result = constants.EXPORT_BLOCKS_LIST ?
-      this.getBlocksListInterval() :
-      await nextIntervalCalculator(this);
+    const requestIntervals = await nextIntervalCalculator(this);


I agree, let's integrate it again in a next PR not to slow down this one. It would be very similar to what you do in nextIntervalCalculator currently - generate a sequence of intervals with the parallelism in mind.

YordanPavlov · 2023-11-29T07:08:56Z

blockchains/eth/eth_worker.js

+      try {
+        const response = await this.ethClient.request(...params);
+        if (response.error || response.result === null) {
+          retries++;


I would expect MAX_RETRIES to specify the number of retries, that is attempts to re-send after the initial sending. Instead if it is is 0, we would not send the request at all. If it is 1, we would send one time, on error wait, but then not retry. Seems to me like the logic is a bit off.

YordanPavlov · 2023-11-29T07:10:16Z

blockchains/eth/lib/next_interval_calculator.js

+  worker.sleepTimeMsec = 0;
+  const progressDifference = worker.lastConfirmedBlock - worker.lastExportedBlock;
+  const maxInterval = constants.MAX_CONCURRENT_REQUESTS * constants.BLOCK_INTERVAL;
+  let intervalArrayLength;


It's not about flexing, const variables make it easier to reason about the code. Minor ofc, your call.

spiderjako added the WIP label Nov 18, 2023

spiderjako force-pushed the add-parallelism-eth branch 4 times, most recently from d27322b to 059c34f Compare November 20, 2023 12:50

spiderjako force-pushed the add-parallelism-eth branch from 059c34f to 28838a0 Compare November 27, 2023 16:15

Lyudmil Danailov added 6 commits November 27, 2023 18:16

Adding parallelism to the ETH pipeline

7f34290

Temporarily removing tests folder

d34c8d4

Adding retry block in requests

9e9c481

Rework of other pipelines

51d867a

Returning eth and erc20 tests, changed next_interval_calculator tests

3d6868d

Minor tweaks

59df0b0

spiderjako force-pushed the add-parallelism-eth branch from 28838a0 to 59df0b0 Compare November 27, 2023 16:19

Lyudmil Danailov added 3 commits November 28, 2023 09:08

Adding a test to the NIC tests

684f5ec

Adding a clearer documentation to NIC

8338b70

Returning a test that ive deleted

de69207

spiderjako removed the WIP label Nov 28, 2023

spiderjako marked this pull request as ready for review November 28, 2023 07:21

spiderjako changed the title ~~Adding parallelism to the ETH pipeline~~ Adding parallelism to the ETH, ERC20, MATIC pipelines Nov 28, 2023

YordanPavlov reviewed Nov 28, 2023

View reviewed changes

Addressing comments

34d7660

WonderBeat approved these changes Nov 28, 2023

View reviewed changes

YordanPavlov approved these changes Nov 29, 2023

View reviewed changes

spiderjako mentioned this pull request Dec 22, 2023

Improving on the parallelism idea #167

Closed

Switching spread->for loop

a63469b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding parallelism to the ETH, ERC20, MATIC pipelines #162

Adding parallelism to the ETH, ERC20, MATIC pipelines #162

spiderjako commented Nov 18, 2023

YordanPavlov Nov 28, 2023

spiderjako Nov 28, 2023

YordanPavlov Nov 28, 2023

YordanPavlov Nov 28, 2023

spiderjako Nov 28, 2023

YordanPavlov Nov 28, 2023

YordanPavlov Nov 28, 2023

YordanPavlov Nov 28, 2023

WonderBeat Nov 28, 2023 •

edited

Loading

spiderjako Nov 28, 2023

YordanPavlov Nov 29, 2023

YordanPavlov Nov 28, 2023

spiderjako Nov 28, 2023

YordanPavlov Nov 28, 2023

spiderjako Nov 28, 2023 •

edited

Loading

YordanPavlov Nov 29, 2023

WonderBeat left a comment

WonderBeat Nov 28, 2023 •

edited

Loading

WonderBeat Nov 28, 2023

YordanPavlov Nov 28, 2023

YordanPavlov Nov 29, 2023

YordanPavlov Nov 29, 2023

Adding parallelism to the ETH, ERC20, MATIC pipelines #162

Are you sure you want to change the base?

Adding parallelism to the ETH, ERC20, MATIC pipelines #162

Conversation

spiderjako commented Nov 18, 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

WonderBeat Nov 28, 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

spiderjako Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WonderBeat left a comment

Choose a reason for hiding this comment

WonderBeat Nov 28, 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

WonderBeat Nov 28, 2023 •

edited

Loading

spiderjako Nov 28, 2023 •

edited

Loading

WonderBeat Nov 28, 2023 •

edited

Loading