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

Refactor IR #2410

Open
notimetoname opened this issue Jun 28, 2023 · 2 comments
Open

Refactor IR #2410

notimetoname opened this issue Jun 28, 2023 · 2 comments
Labels
discussion Open discussion of some problem enhancement Improving existing functionality help wanted Extra attention is needed I4 No visible changes neofs-ir Inner Ring node application issues S1 Highly significant U4 Nothing urgent

Comments

@notimetoname
Copy link
Contributor

Some questions:

  1. Why does it have workers, starters and runners? Also, all of them are anonymous, it makes it difficult to understand what "services" (well, just func ()) are running/will be running.
  2. Why does it read config everywhere, everywhere, everywhere, etc it wants? SN has been using a separate place for config reading for years already. After config is parsed it passes (and modifies its types if need be) in every component that requires it.
  3. Its constructor is 650+ lines long. It does many things (converting types, creating new types, reading configs, making some RPC, logging, filling DB, deciding the full IR work mode in a huge if ... else, etc). I mean that is not bad that everything is happening at the New step, I mean that most of that is done literally inside that func.
  4. Drop deprecated Notifications use from the morph client #2309 will require changes that should drop processors, listeners all related things. Drop deprecated Notifications use from the morph client #2309 says "Each service can do Receive* for things it needs..." but IR has only two services in fact: timers (not services TBH, they only can accept Tick calls and be Reseted), and a Listener that should be dropped. It also makes anything related to Restart services after switching to another neo-go RPC node  #1337 hard to implement easily.
@notimetoname notimetoname added help wanted Extra attention is needed discussion Open discussion of some problem neofs-ir Inner Ring node application issues refactor labels Jun 28, 2023
@roman-khimov
Copy link
Member

The way I see it working is:

  • IR main reads config and keeps it internally forever
  • it creates connections
  • it passes them to innerring.New()
  • which can do whatever it needs to initialialize (read various data) and set up event listeners
  • it works until some connection breaks
  • when it breaks IR stops completely (including additional goroutines if any)
  • main kicks in, reconnects to RPC
  • and restarts IR again

The rationale is rather simple: IR state is completely stored on blockchain, it's a service that doesn't differ much from Oracle or Notary services, so it MUST NOT deal with connections internally at all, it should take them as a given. It also shouldn't care about configuration files, these are cmd things by definition.

@cthulhu-rider
Copy link
Contributor

if Inner Ring is based on and works around blockchain, then in my mind it should be constructed from type Blockchain interface providing all IR-expected stuff. From IR pov, Blockchain wont deal with connections (no endpoint, conn parameters).

@roman-khimov roman-khimov added enhancement Improving existing functionality U4 Nothing urgent S2 Regular significance I4 No visible changes S1 Highly significant and removed refactor S2 Regular significance labels Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Open discussion of some problem enhancement Improving existing functionality help wanted Extra attention is needed I4 No visible changes neofs-ir Inner Ring node application issues S1 Highly significant U4 Nothing urgent
Projects
None yet
Development

No branches or pull requests

3 participants