Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Send client state to cluster in re-connections[API-1644] #1415

Merged
merged 13 commits into from
Nov 28, 2022
Merged

Send client state to cluster in re-connections[API-1644] #1415

merged 13 commits into from
Nov 28, 2022

Conversation

harunalpak
Copy link
Contributor

@harunalpak harunalpak commented Nov 17, 2022 •
edited by srknzl
Loading

@codecov
Copy link

codecov bot commented Nov 17, 2022 •
edited
Loading

Codecov Report

Merging #1415 (1f7388e) into master (871a812) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1415      +/-   ##
==========================================
- Coverage   93.17%   93.14%   -0.03%     
==========================================
  Files         465      465              
  Lines       16430    16431       +1     
  Branches     1337     1337              
==========================================
- Hits        15308    15305       -3     
- Misses        823      825       +2     
- Partials      299      301       +2
Impacted Files Coverage Δ
src/network/ConnectionManager.ts 79.62% <100.00%> (+0.04%) ⬆️
src/network/Connection.ts 94.22% <0.00%> (-0.89%) ⬇️
src/util/Util.ts 86.30% <0.00%> (-0.69%) ⬇️
src/invocation/InvocationService.ts 95.29% <0.00%> (-0.40%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

srknzl
srknzl requested changes Nov 18, 2022
View reviewed changes
Copy link
Member

@srknzl srknzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a test? We cannot add split brain tests for obvious reasons but, can we add a test that will check if state is sent to the cluster after a reconnect? (could be in ClientReconnectTest, via using sinon spies/fakes)

@harunalpak
Copy link
Contributor Author

harunalpak commented Nov 25, 2022 •
edited
Loading

Should we add a test? We cannot add split brain tests for obvious reasons but, can we add a test that will check if state is sent to the cluster after a reconnect? (could be in ClientReconnectTest, via using sinon spies/fakes)

Yes we should add this test to ensure that whether the state sent to cluster or not. I will add the test and send to PR.

srknzl
srknzl reviewed Nov 28, 2022
View reviewed changes
test/integration/backward_compatible/serial/ClientReconnectTest.js
'hazelcast.client.heartbeat.timeout': 3000
}
});
await RC.terminateMember(cluster.id, member.uuid);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await RC.terminateMember(cluster.id, member.uuid);
await TestUtil.waitForConnectionCount(client, 1);
await RC.terminateMember(cluster.id, member.uuid);

@harunalpak harunalpak merged commit 60db202 into hazelcast:master Nov 28, 2022
@harunalpak harunalpak added this to the 5.2.0 milestone Nov 28, 2022
harunalpak added a commit to harunalpak/hazelcast-nodejs-client that referenced this pull request Dec 8, 2022
@harunalpak
Send client state to cluster in re-connections[API-1644] (hazelcast#1415
09d46dd 
)

* Make ReadResultSet iterable [API-1315]

* Make ReadResultSet iterable [API-1315]

* Changes made according to comments on PR.[API-1315]

* Send client state to cluster in re-connections [API-1644]

* New test added related to check InitializeClientOnCluster

* New test added

* lint problem fixed
@mdumandag mdumandag mentioned this pull request Dec 8, 2022
Closed
mdumandag
mdumandag reviewed Dec 8, 2022
View reviewed changes
test/integration/backward_compatible/serial/ClientReconnectTest.js
@@ -59,6 +40,37 @@ describe('ClientReconnectTest', function () {
await testFactory.shutdownAll();
});

it('should send the client state to the cluster after reconnections, ' +
+'regardless it is connected back to possibly the same cluster with the same id or not.', async function () {
const fakeInitializeClientOnCluster = sandbox.replace(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: This test will probably fail in backward compatibility tests as there is no client version filter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harunalpak I overlooked this, can you add a version check?

srknzl pushed a commit to fatihozer0/hazelcast-nodejs-client that referenced this pull request Mar 24, 2023
@harunalpak @srknzl
Send client state to cluster in re-connections[API-1644] (hazelcast#1415
39d7124 
)

* Make ReadResultSet iterable [API-1315]

* Make ReadResultSet iterable [API-1315]

* Changes made according to comments on PR.[API-1315]

* Send client state to cluster in re-connections [API-1644]

* New test added related to check InitializeClientOnCluster

* New test added

* lint problem fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdumandag mdumandag mdumandag left review comments

@srknzl srknzl srknzl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@harunalpak @mdumandag @srknzl