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

Routing: priority and mandatory Router Params #9083

Closed
ajnavarro opened this issue Jul 5, 2022 · 8 comments
Closed

Routing: priority and mandatory Router Params #9083

ajnavarro opened this issue Jul 5, 2022 · 8 comments
Labels
kind/feature A new feature

Comments

@ajnavarro
Copy link
Member

ajnavarro commented Jul 5, 2022

Description

Coming from this discussion: #8997 (comment) we need to figure out if we need and how to add a way to tweak router behavior.

  • priority – allows setting the order (when it matters)
  • mandatory – allows for marking some routers as mandatory and others as best-effort
@ajnavarro ajnavarro added the kind/feature A new feature label Jul 5, 2022
@BigLep BigLep changed the title Delegated Routing: priority and mandatory Router Params Routing: priority and mandatory Router Params Jul 25, 2022
@BigLep
Copy link
Contributor

BigLep commented Jul 25, 2022

@ajnavarro : thanks for creating this issue. A few thing:

  1. I removed "Delegated" from the title. I think this is about routing configurability in general.
  2. I think we should update the issue to expand on the usecases we want to support. For example, mandatory makes sense to me. I assume that means that we don't return until all mandatory routers have returned (or timedout). Otherwise we return as soon as we get a response. Priority isn't clear to me.
  3. Can we put together a proposal for what the config of this will look like?

@ajnavarro
Copy link
Member Author

ajnavarro commented Jul 26, 2022

Right now we have the following JSON format for routing configuration:

"Routers" : {
		"CidContact" : {
			"Type" : "reframe",
			"Enabled" : true
			"Parameters" : {
				"Address" : "<URL>"
				}
		}
}

Proposal:

  • Add a new Mandatory flag (false by default).
  • In my opinion, I would implement priority by order (this will simplify implementation and the configuration file). Even if maps in Go have no guaranteed order, we can parse the actual Routers format in a custom way to allow ordering.
"Routers" : {
		"CidContact1" : {
			"Type" : "reframe",
			"Enabled" : true,
			"Mandatory": true,
			"Parameters" : {
				"Address" : "<URL>"
				}
		},
       "CidContact2" : {
			"Type" : "dht",
			"Enabled" : false,
			"Parameters" : {
				"PARAM" : "VAL"
				}
		}
}

Internally, Routers will change from:

type Routing struct {
	Type *OptionalString `json:",omitempty"`
	Routers map[string]Router
}
type Router struct {
	Type string
	Enabled Flag `json:",omitempty"`
	Parameters map[string]string
}

To:

type Routing struct {
	Type *OptionalString `json:",omitempty"`
	Routers []Router
}
type Router struct {
	Type string
    Name string
	Enabled Flag `json:",omitempty"`
    Mandatory Flag `json:",omitempty"`
	Parameters map[string]string
}

The major workload here is not in the configuration changes, but in the new behavior that we will need to implement to support ordering and mandatory routers.

Right now we are using github.com/libp2p/go-libp2p-routing-helpers library, specifically the Tiered router to use all routers together. We need to make our own implementation of Tiered, supporting mandatory routers.

@BigLep
Copy link
Contributor

BigLep commented Jul 26, 2022

@ajnavarro : thanks for the extra details. A few things on this:

  1. Why does order matter? Is this because we aren't requesting all the routers in parallel? Or is this about which router's results to prioritize?
  • If we aren't doing everything in parallel, then how long do we wait before we start executing the next in the priority list?
  1. How are results from multiple mandatory routers combined? This seems like an important thing to document.
  2. Where does someone specify the timeout value for a given router?

My initial thought is that we either:

  1. Keep this simple with logic like: "all routers are executed in parallel; we return as soon as all of the mandatory routers have returned or timed out; everything else that isn't mandatory has its response included if it completed in time; results are added to an array in the order corresponding with the router.

OR if that's good enough

  1. Go the full configurability route where you can have recursive "chains of routers" with different "strategies" for when to execute, how to combine results, etc. I assume there are good patterns we can follow for this, but am hopeful we don't need to go down this higher complexity path right now.

@BigLep BigLep moved this to 🥞 Todo in IPFS Shipyard Team Jul 26, 2022
@ajnavarro
Copy link
Member Author

Why does order matter?

It matters depending on how we implement the Routers Wrapper. Right now the actual behavior is:

  1. Execute method on all Routers in parallel. It fails if one fails:
    • PutValue
    • Provide
    • Bootstrap
  2. Execute GetValue on one Router at a time. Return the first non-nil result and stop the iteration.
    • GetValue
    • GetPublicKey
    • FindPeer
  3. Execute SearchValue on all Routers in parallel and mix the results.
    • SearchValue
    • FindProvidersAsync

All of these methods are using Context, so we have a timeout to stop the execution if some router is taking too long or we didn't have time to execute all of them.

How are results from multiple mandatory routers combined?

Maybe we need to define better what means mandatory. For me, mandatory will cause the execution to fail if it fails, but I don't know if it is the best thing to do or if it is useful to the user.

Where does someone specify the timeout value for a given router?

If we need it as a specific value per router, we can add it as a parameter (Parameters map[string]string), like reframe URL param.

I would go with option number 1, keeping things simple, only taking into account that not all methods are running in parallel (should we change actual method behaviors?). Do we know about specific use cases or personas?

@BigLep
Copy link
Contributor

BigLep commented Aug 12, 2022

@ajnavarro : sorry not to know. Just for my own learning, can you please link me to the "Routers Wrapper" as it's implemented today?

@BigLep
Copy link
Contributor

BigLep commented Aug 12, 2022

@ajnavarro : sorry not to know. Just for my own learning, can you please link me to the "Routers Wrapper" as it's implemented today?

Per 2022-08-12 verbal conversation, https://github.com/libp2p/go-libp2p-routing-helpers is what is being referred to here.

@ajnavarro
Copy link
Member Author

Sorry for the delay @BigLep . We are specifically using https://github.com/libp2p/go-libp2p-routing-helpers/blob/master/tiered.go#L18

@ajnavarro
Copy link
Member Author

ajnavarro commented Aug 23, 2022

Superseded by #9157 , #9188 and libp2p/go-libp2p-routing-helpers#56

Repository owner moved this from 🥞 Todo to 🎉 Done in IPFS Shipyard Team Aug 23, 2022
@BigLep BigLep moved this from 🎉 Done to ☑️ Done (Archive) in IPFS Shipyard Team Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A new feature
Projects
No open projects
Archived in project
Development

No branches or pull requests

2 participants