-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Stateful plugin loading #4806
Stateful plugin loading #4806
Conversation
9985522
to
0349fb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicks, overall LGTM.
This could be used to solve the config problem from #4506 (comment) in a quite non-hacky way.
I wonder what impact this will have on the command execution perf as afaik we don't load plugins when executing commands on the daemon. On the other hand we might end up doing that anyways if command-plugins ever become a thing.
cmd/ipfs/main.go
Outdated
@@ -112,6 +112,31 @@ func mainRet() int { | |||
} | |||
log.Debugf("config path is %s", repoPath) | |||
|
|||
pluginpath := filepath.Join(repoPath, "plugins") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could wrap this whole code block into a separate function like loadPlugins
cmd/ipfs/main.go
Outdated
} | ||
|
||
err = plugins.Initialize() | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a function returns only a single error you can collapse it into if err := plugins.Initialize(); err != nil {
core/builder.go
Outdated
@@ -15,6 +15,7 @@ import ( | |||
dag "github.com/ipfs/go-ipfs/merkledag" | |||
resolver "github.com/ipfs/go-ipfs/path/resolver" | |||
pin "github.com/ipfs/go-ipfs/pin" | |||
"github.com/ipfs/go-ipfs/plugin/loader" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed prefix
The The problem we are having here is: want want to use plugins pre and post commands lib execution. Best course of action I see here is adding This way the plugins can be loaded before command execution and then used latter with created @Stebalien what do you think? |
e51dee1
to
fbbf41c
Compare
fbbf41c
to
6c3879b
Compare
cc @frrist |
It might help with Tracing API as you will be able to access config, just at latter stage after initial |
The main reason for needing access to the nodes config was to determine the peerId of the node running the plugin, this was how we were going to name the tracer. |
Yeah, i think after this, you could create like ConfigConsumer interface, implement it on the tracing plugin, then create a method that provides the config for all the plugins that implement that interface and you should be good. |
@PlayerWithoutName could you rebase your PR. To solve conflicts with other PR (#4506 ) please base it on |
0f40a97
to
aea981a
Compare
commands/request.go
Outdated
@@ -18,6 +19,7 @@ type Context struct { | |||
Online bool | |||
ConfigRoot string | |||
ReqLog *ReqLog | |||
Plugins *loader.PluginLoader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd separate this with a newline from the group above.
plugin/loader/loader.go
Outdated
} | ||
// Initialize all the loaded plugins | ||
func (loader *PluginLoader) Initialize() error { | ||
return initialize(loader.plugins) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just put these funcs on the struct directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from 2 nitpicks that @magik6k pointed out SGWM.
2bb74de
to
8b45855
Compare
plugin/loader/initializer.go
Outdated
func run(plugins []plugin.Plugin) error { | ||
for _, pl := range plugins { | ||
//Run the plugins | ||
func (loader *PluginLoader) Run() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is good practice to keep the implementation of given structure in one package.
In this case, it means keeping those functions in one file with the definition of the struct.
8b45855
to
424ccf7
Compare
@Stebalien want to review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I do worry about the perf implications of loading plugins on the client but:
- Most plugins should do most of their work on-demand anyways.
- Ideally, we'd want to run IPLD plugins on the client.
@Kubuxu this one needs a rebase. Then i'll merge it in |
424ccf7
to
abdfeca
Compare
Rebased |
cmd/ipfs/main.go
Outdated
@@ -47,6 +47,44 @@ const ( | |||
heapProfile = "ipfs.memprof" | |||
) | |||
|
|||
type cmdInvocation struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like some code that got deleted in another PR is getting reintroduced here. cc @magik6k
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, it was legacy code accidentally put up with second commit.
abdfeca
to
1b7aaf8
Compare
1b7aaf8
to
5bb935f
Compare
5bb935f
to
802521b
Compare
Rebased... will merge if I can get the tests to pass. |
License: MIT Signed-off-by: Kacper Łukawski <kacluk98@gmail.com>
802521b
to
a1e654c
Compare
Depends on #4506
The plugins are currently loaded during Executor creation, which takes some extensibility from them. This PR makes them 'stateful', which will allow for more flexibility.