Skip to content

Commit 259f745

Browse files
ivaylogarnev-limechainivaylogarnev-limechain
authored
chore(fix): Hitting unhealthy node 10 times (#2819)
* fix: Refactor the unhealthy logic inside the Executable class and introduces excludeCurrent method in the List class Signed-off-by: ivaylogarnev-limechain <[email protected]> * fix: Added nodeAccountIds current node increasement Signed-off-by: ivaylogarnev-limechain <[email protected]> * refactor: Moved the nodeAccountIds advance outside condition Signed-off-by: ivaylogarnev-limechain <[email protected]> * refactor: Changed and simplified the logic for the unhealthy nodes and added unit tests Signed-off-by: ivaylogarnev-limechain <[email protected]> * test: Added mmultiple nodes UNAVAILABLE behavior test in AccountInfoMocking Signed-off-by: ivaylogarnev-limechain <[email protected]> * refactor: Changed the mocker response node account id Signed-off-by: ivaylogarnev-limechain <[email protected]> * fix: Removed unhealthy node from ClientIntegration test suite Signed-off-by: ivaylogarnev-limechain <[email protected]> --------- Signed-off-by: ivaylogarnev-limechain <[email protected]>
1 parent 1a4392d commit 259f745

File tree

4 files changed

+124
-35
lines changed

4 files changed

+124
-35
lines changed

‎src/Executable.js

Lines changed: 15 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -424,20 +424,6 @@ export default class Executable {
424424
throw new Error("not implemented");
425425
}
426426

427-
/**
428-
* Advance the request to the next node
429-
*
430-
* FIXME: This method used to perform different code depending on if we're
431-
* executing a query or transaction, but that is no longer the case
432-
* and hence could be removed.
433-
*
434-
* @protected
435-
* @returns {void}
436-
*/
437-
_advanceRequest() {
438-
this._nodeAccountIds.advance();
439-
}
440-
441427
/**
442428
* Determine if we should continue the execution process, error, or finish.
443429
*
@@ -659,29 +645,31 @@ export default class Executable {
659645
const channel = node.getChannel();
660646
const request = await this._makeRequestAsync();
661647

662-
// advance the internal index
663-
// non-free queries and transactions map to more than 1 actual transaction and this will cause
664-
// the next invocation of makeRequest to return the _next_ transaction
665-
// FIXME: This is likely no longer relavent after we've transitioned to using our `List` type
666-
// can be replaced with `this._nodeAccountIds.advance();`
667-
this._advanceRequest();
668-
669648
let response;
670649

671-
// If the node is unhealthy, wait for it to be healthy
672-
// FIXME: This is wrong, we should skip to the next node, and only perform
673-
// a request backoff after we've tried all nodes in the current list.
674-
if (!node.isHealthy() && this._nodeAccountIds.length > 1) {
650+
if (!node.isHealthy()) {
651+
const isLastNode =
652+
this._nodeAccountIds.index ===
653+
this._nodeAccountIds.list.length - 1;
654+
655+
if (isLastNode || this._nodeAccountIds.length <= 1) {
656+
throw new Error(
657+
`Network connectivity issue: All nodes are unhealthy. Original node list: ${this._nodeAccountIds.list.join(", ")}`,
658+
);
659+
}
660+
675661
if (this._logger) {
676662
this._logger.debug(
677-
`[${logId}] node is not healthy, skipping waiting ${node.getRemainingTime()}`,
663+
`[${logId}] Node is not healthy, trying the next node.`,
678664
);
679665
}
680666

681-
// We don't need to wait, we can proceed to the next attempt.
667+
this._nodeAccountIds.advance();
682668
continue;
683669
}
684670

671+
this._nodeAccountIds.advance();
672+
685673
try {
686674
// Race the execution promise against the grpc timeout to prevent grpc connections
687675
// from blocking this request

‎test/integration/AccountBalanceIntegrationTest.js

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,7 @@ describe("AccountBalanceQuery", function () {
2626
expect(balance.hbars.toTinybars().compare(0)).to.be.equal(1);
2727
});
2828

29-
// TODO(2023-11-01 NK) - test is consistently failing and should be enabled once fixed.
30-
// eslint-disable-next-line mocha/no-skipped-tests
31-
xit("can connect to previewnet with TLS", async function () {
29+
it("can connect to previewnet with TLS", async function () {
3230
if (skipTestDueToNodeJsVersion(16)) {
3331
return;
3432
}

‎test/integration/ClientIntegrationTest.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,8 @@ describe("ClientIntegration", function () {
151151
"0.testnet.hedera.com:50211": new AccountId(3),
152152
"34.94.106.61:50211": new AccountId(3),
153153
"50.18.132.211:50211": new AccountId(3),
154-
"138.91.142.219:50211": new AccountId(3),
154+
// IP address currently not responding
155+
// "138.91.142.219:50211": new AccountId(3)
155156
};
156157

157158
const clientForNetwork = Client.forNetwork(nodes);

‎test/unit/AccountInfoMocking.js

Lines changed: 106 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ const ACCOUNT_INFO_QUERY_RESPONSE = {
3030
accountID: {
3131
shardNum: Long.ZERO,
3232
realmNum: Long.ZERO,
33-
accountNum: Long.fromNumber(10),
33+
accountNum: Long.fromNumber(3),
3434
},
3535
key: {
3636
ed25519: PRIVATE_KEY.publicKey.toBytesRaw(),
@@ -92,7 +92,7 @@ describe("AccountInfoMocking", function () {
9292
.setAccountId("0.0.3")
9393
.execute(client);
9494

95-
expect(info.accountId.toString()).to.be.equal("0.0.10");
95+
expect(info.accountId.toString()).to.be.equal("0.0.3");
9696
});
9797

9898
it("should retry on UNAVAILABLE", async function () {
@@ -108,7 +108,7 @@ describe("AccountInfoMocking", function () {
108108
.setAccountId("0.0.3")
109109
.execute(client);
110110

111-
expect(info.accountId.toString()).to.be.equal("0.0.10");
111+
expect(info.accountId.toString()).to.be.equal("0.0.3");
112112
});
113113

114114
it("should error when cost is greater than max cost set on client", async function () {
@@ -255,7 +255,7 @@ describe("AccountInfoMocking", function () {
255255
.setAccountId("0.0.3")
256256
.execute(client);
257257

258-
expect(info.accountId.toString()).to.be.equal("0.0.10");
258+
expect(info.accountId.toString()).to.be.equal("0.0.3");
259259
});
260260

261261
it("should be able to execute after getting transaction hashes", async function () {
@@ -591,4 +591,106 @@ describe("AccountInfoMocking", function () {
591591
);
592592
}
593593
});
594+
595+
it("should demonstrate UNAVAILABLE behavior with multiple nodes", async function () {
596+
({ client, servers } = await Mocker.withResponses([
597+
// First node (0.0.3)
598+
[
599+
{ error: UNAVAILABLE },
600+
{ error: UNAVAILABLE },
601+
{ error: UNAVAILABLE },
602+
],
603+
// Second node (0.0.4)
604+
[
605+
{ error: UNAVAILABLE },
606+
{ response: ACCOUNT_INFO_QUERY_COST_RESPONSE },
607+
{ response: ACCOUNT_INFO_QUERY_RESPONSE },
608+
],
609+
]));
610+
611+
const info = await new AccountInfoQuery()
612+
.setNodeAccountIds([new AccountId(3), new AccountId(4)])
613+
.setAccountId("0.0.3")
614+
.execute(client);
615+
616+
expect(info.accountId.toString()).to.be.equal("0.0.3");
617+
});
618+
619+
describe("Node health checks", function () {
620+
beforeEach(async function () {
621+
const responses1 = [
622+
{ response: ACCOUNT_INFO_QUERY_COST_RESPONSE },
623+
{ response: ACCOUNT_INFO_QUERY_RESPONSE },
624+
];
625+
626+
const responses2 = [
627+
{ response: ACCOUNT_INFO_QUERY_COST_RESPONSE },
628+
{ response: ACCOUNT_INFO_QUERY_RESPONSE },
629+
];
630+
631+
const responses3 = [
632+
{ response: ACCOUNT_INFO_QUERY_COST_RESPONSE },
633+
{ response: ACCOUNT_INFO_QUERY_RESPONSE },
634+
];
635+
636+
({ client, servers } = await Mocker.withResponses([
637+
responses1,
638+
responses2,
639+
responses3,
640+
]));
641+
});
642+
643+
it("should throw error because every node is unhealthy", async function () {
644+
// Make node 3 unhealthy
645+
client._network._network.get("0.0.3")[0]._readmitTime =
646+
Date.now() + 100 * 10010;
647+
648+
// Make node 4 unhealthy
649+
client._network._network.get("0.0.4")[0]._readmitTime =
650+
Date.now() + 100 * 10010;
651+
652+
try {
653+
await new AccountInfoQuery()
654+
.setNodeAccountIds([new AccountId(3), new AccountId(4)])
655+
.setAccountId("0.0.3")
656+
.execute(client);
657+
throw new Error("should have thrown");
658+
} catch (error) {
659+
expect(error.message).to.equal(
660+
"Network connectivity issue: All nodes are unhealthy. Original node list: 0.0.3, 0.0.4",
661+
);
662+
}
663+
});
664+
665+
it("should skip unhealthy node and execute with healthy node", async function () {
666+
// Make node 3 unhealthy
667+
client._network._network.get("0.0.3")[0]._readmitTime =
668+
Date.now() + 100 * 10010;
669+
670+
const info = await new AccountInfoQuery()
671+
.setNodeAccountIds([new AccountId(3), new AccountId(4)])
672+
.setAccountId("0.0.3")
673+
.execute(client);
674+
675+
expect(info.accountId.toString()).to.be.equal("0.0.3");
676+
});
677+
678+
it("should execute with last healthy node when multiple nodes are unhealthy", async function () {
679+
// Make nodes 3 and 5 unhealthy
680+
client._network._network.get("0.0.3")[0]._readmitTime =
681+
Date.now() + 100 * 10010;
682+
client._network._network.get("0.0.5")[0]._readmitTime =
683+
Date.now() + 100 * 10010;
684+
685+
const info = await new AccountInfoQuery()
686+
.setNodeAccountIds([
687+
new AccountId(3),
688+
new AccountId(4),
689+
new AccountId(5),
690+
])
691+
.execute(client);
692+
693+
expect(info.accountId.toString()).to.be.equal("0.0.3");
694+
});
695+
});
594696
});

0 commit comments

Comments
 (0)