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.

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

introduce entity that will keep info about services current state and integrate it into DASer and Syncer #725

Closed

Conversation

vgonkivs
Copy link
Member

Resolves #724

@vgonkivs vgonkivs requested a review from renaynay May 17, 2022 12:58
libs/utils/state_wrapper.go Outdated Show resolved Hide resolved
libs/utils/state_wrapper.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #725 (9623bb5) into main (90ba117) will decrease coverage by 0.02%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main     #725      +/-   ##
==========================================
- Coverage   52.55%   52.52%   -0.03%     
==========================================
  Files         112      113       +1     
  Lines        6405     6425      +20     
==========================================
+ Hits         3366     3375       +9     
- Misses       2688     2696       +8     
- Partials      351      354       +3     
Impacted Files Coverage Δ
das/daser.go 69.37% <42.85%> (-1.22%) ⬇️
header/sync/sync.go 64.59% <63.63%> (-0.76%) ⬇️
libs/utils/state_wrapper.go 100.00% <100.00%> (ø)
header/core/listener.go 51.92% <0.00%> (-5.77%) ⬇️

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 90ba117...9623bb5. Read the comment docs.

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Making Service like this libs/service.Service and usage like :

type Syncer struct{
	service.Service
	...
}

func (s *Syncer) Stop(context.Context) error {
	if s.state.State() == service.Stopped {
		return nil
	}
	defer s.state.SetState(service.Stopped)
}

WDYT?

das/daser.go Outdated Show resolved Hide resolved
libs/utils/state_wrapper.go Outdated Show resolved Hide resolved
@Wondertan
Copy link
Member

Other thoughts:

  • As we discussed, this should be part of FX, and FX can track on its own the state of each Service that was started or not. No matter how we try to solve this problem, it will be better solved by the uber-go/fx and their team. We need deeper integration with the dependency graph to not just stop a Service but also stop everything that depends on the stopped service. We should make a proposal as I did in [Feature Request] OverrideSupply and OverrideProvide  uber-go/fx#825 for this.
  • @vgonkivs, can you tell me if you have other plans for this structure? It feels like it is a bit of overengineering for a simple atomic switch. If there are other long term plans/ideas on how we can use this, then I am fine to keep it, but if not, I see no reason to keep it, considering the proper solution should be done on the FX side.

@vgonkivs
Copy link
Member Author

@Wondertan, no other plans except provided. I do not think that it's overengineering. It allows to re-use this structure in different services and provides simple aliases for On/Off state

@vgonkivs vgonkivs force-pushed the add_flag_for_stopped_services branch from 9623bb5 to 08f4472 Compare May 17, 2022 13:55
@vgonkivs vgonkivs changed the title utils: add state wrapper add state service May 17, 2022
@vgonkivs vgonkivs requested a review from Wondertan May 17, 2022 14:00
@Wondertan
Copy link
Member

Can you also rename the PR to something more appropriate? We already have a state service that accesses the State of the blockchain

@@ -82,6 +86,11 @@ func (d *DASer) Start(context.Context) error {

// Stop stops sampling.
func (d *DASer) Stop(ctx context.Context) error {
if d.State() == service.Stopped {
log.Debug("Daser is stopped")
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
log.Debug("Daser is stopped")
log.Debug("DASer is stopped")


// Service provides an interface to change an check service state
type Service struct {
state uint32
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
state uint32
state State

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case storing and loading state will be more complex:

* atomic.StoreUint32((*uint32)(&s.state), uint32(state))
* return State(atomic.LoadUint32((*uint32)(&s.state)))

IMO, better to leave uint32

Copy link
Member

Choose a reason for hiding this comment

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

Then maybe the naming of state here is inconsistent? s.state seems to be a key for the state, not a state? If so, it it's 1) not clearly named or documented, and 2) should be uint64, unless there's a very specific reason it must be a uint32.

@vgonkivs vgonkivs changed the title add state service introduce entity that will keep info about services current state and integrate it into DASer and Syncer May 17, 2022
@vgonkivs vgonkivs requested a review from adlerjohn May 17, 2022 19:38
@vgonkivs
Copy link
Member Author

Closing this PR in favour of more priority tasks.

@vgonkivs vgonkivs closed this May 18, 2022
@vgonkivs vgonkivs deleted the add_flag_for_stopped_services branch January 9, 2023 13:46
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.

service: Add flag for stopped services
5 participants