Skip to content

Fix Invocation Service group lost handling#816

Merged
yuce merged 3 commits intohazelcast:masterfrom
yuce:fix-invocation-grouplost-sub
Jul 4, 2022
Merged

Fix Invocation Service group lost handling#816
yuce merged 3 commits intohazelcast:masterfrom
yuce:fix-invocation-grouplost-sub

Conversation

@yuce
Copy link
Copy Markdown
Collaborator

@yuce yuce commented Jul 3, 2022

This PR fixes group lost event handling in the Invocation Service:

  • The subscriber to the group lost event in the invocation service should return if the invocation service is stopped.
  • handleGroupLost relied on an atomic value for protection, which was wrong. Replaced that with a mutex.

…ld return if the invocation service is stopped.
@yuce yuce requested a review from utku-caglayan July 3, 2022 07:41
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 3, 2022

Codecov Report

Merging #816 (21c0088) into master (69a3310) will decrease coverage by 0.01%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master     #816      +/-   ##
==========================================
- Coverage   77.13%   77.11%   -0.02%     
==========================================
  Files         329      329              
  Lines       15547    15556       +9     
==========================================
+ Hits        11992    11996       +4     
- Misses       2709     2713       +4     
- Partials      846      847       +1     
Impacted Files Coverage Δ
internal/invocation/invocation_service.go 79.26% <94.11%> (-0.09%) ⬇️
internal/cluster/cluster_service.go 75.87% <0.00%> (-1.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69a3310...21c0088. Read the comment docs.

@yuce yuce changed the title Fix Invocation Service group lost subscriber Fix Invocation Service group lost handling Jul 3, 2022
Copy link
Copy Markdown
Contributor

@utku-caglayan utku-caglayan left a comment

Choose a reason for hiding this comment

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

I think this works and understand the reasoning behind adding the doneChannel control. But I do not get replacing the atomic variable with a mutex-protected one. Can you share your reasoning on that one?

@yuce
Copy link
Copy Markdown
Collaborator Author

yuce commented Jul 4, 2022

I think this works and understand the reasoning behind adding the doneChannel control. But I do not get replacing the atomic variable with a mutex-protected one. Can you share your reasoning on that one?

Atomic variable can protect only itself. They can be used to protect a block when used with compare and swap, but otherwise a mutex should be used. Line

if atomic.LoadInt32(&s.state) != ready {
is not enough to protect the block below, since invocation service can stopped right after that if statement.

@nevzatseferoglu nevzatseferoglu self-requested a review July 4, 2022 15:03
Copy link
Copy Markdown
Contributor

@nevzatseferoglu nevzatseferoglu left a comment

Choose a reason for hiding this comment

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

LGTM! Just a quick remark,s.executor.stop() use a waitgroup inside it. In case of any unintended infinite spinning on wait creates a deadlock on the invocation service state mutex.
https://github.com/hazelcast/hazelcast-go-client/blob/master/internal/invocation/stripe_executor.go#L80

@yuce yuce merged commit a5c2607 into hazelcast:master Jul 4, 2022
@yuce yuce deleted the fix-invocation-grouplost-sub branch July 4, 2022 18:50
utku-caglayan pushed a commit to utku-caglayan/hazelcast-go-client that referenced this pull request Jul 5, 2022
Fixed invocation service's group lost event handling

(cherry picked from commit a5c2607)
utku-caglayan added a commit that referenced this pull request Jul 5, 2022
* Fix all partitions being assigned to one node (#775)

Fix all partitions getting mapped to one node

(cherry picked from commit ec11592)

* Fix go tools version so that it works with go 1.15 (#792)

(cherry picked from commit dc17db9)

* [Backports] Fixes SQL driver test to work with SSL

* Fix SQL leak (#795)

* Fixes SQL goroutine leak.

(cherry picked from commit c10fdf4)

* Fix Failover SSL (#797)

Fixes the connection manager not using failover cluster settings for SSL

(cherry picked from commit 989e683)

* [Backports] Fix uptime gauge [API-1382] (#808)

* Fix Invocation Service group lost handling (#816)

Fixed invocation service's group lost event handling

(cherry picked from commit a5c2607)

* Remove MakeSubscriptionID method (#815)

Removed MakeSubscriptionID method

(cherry picked from commit d5fc815)

* Renamed proto.EntrySizeInBytes and removed dead code (#814)

(cherry picked from commit f06827c)

* add missing API usage (#776)

(cherry picked from commit c5e35ff)

* update client version

* fix for client version test

Co-authored-by: Alex Barkan <aleckz@gmail.com>
Co-authored-by: Serkan Özel <serkan.ozel@hazelcast.com>
Co-authored-by: Yüce Tekol <yuce.tekol@hazelcast.com>
@yuce yuce added this to the 1.3.0 milestone Jul 7, 2022
yuce added a commit that referenced this pull request Dec 26, 2022
Fixed invocation service's group lost event handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants