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

feat: Shared Plugin RPC Host #3238

Merged
merged 76 commits into from
Jan 3, 2023
Merged

feat: Shared Plugin RPC Host #3238

merged 76 commits into from
Jan 3, 2023

Conversation

joshLong145
Copy link
Contributor

  • Adds functionality to allow plugins to share a host if executing in the same context (loaded from the same configuration)
  • New plugin config flag has been added to enable this feature
sharedHost: true

Note: If the flag is not set no plugins will share a rpc server, keeping a 1 - 1 relationship between rpc client and server.

  • Addition of pkg/cache to keep plugin host contexts for finding rpc servers in separate ignite processes.

@joshLong145 joshLong145 added the component:extensions Related to Ignite Extensions. label Dec 6, 2022
@joshLong145 joshLong145 marked this pull request as ready for review December 11, 2022 00:47
@joshLong145 joshLong145 requested a review from aljo242 as a code owner December 11, 2022 00:47
@codecov
Copy link

codecov bot commented Dec 26, 2022

Codecov Report

Merging #3238 (17ef117) into main (e2cc76c) will increase coverage by 0.16%.
The diff coverage is 67.56%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3238      +/-   ##
==========================================
+ Coverage   19.90%   20.06%   +0.16%     
==========================================
  Files         384      385       +1     
  Lines       31003    31102      +99     
==========================================
+ Hits         6170     6240      +70     
- Misses      24245    24264      +19     
- Partials      588      598      +10     
Impacted Files Coverage Δ
ignite/cmd/plugin.go 27.84% <0.00%> (ø)
ignite/services/plugin/interface.go 43.58% <ø> (ø)
ignite/services/plugin/cache.go 57.14% <57.14%> (ø)
ignite/services/plugin/plugin.go 71.76% <78.43%> (+2.13%) ⬆️
ignite/services/plugin/scaffold.go 56.75% <100.00%> (+2.47%) ⬆️
ignite/pkg/env/env.go 38.46% <0.00%> (+15.38%) ⬆️

@tbruyelle
Copy link
Contributor

tbruyelle commented Jan 1, 2023

@joshLong145 I refactored a little bit the cache part :

  • use .ignite/plugins as root dir
  • remove usage of intermediary ConfigContext (the tests are passing, so I assume I solved the problem you had with the net.Addr, but maybe you should check again please)
  • ReadPluginConfCache returns a ReattachConfig instead of filling the one in paramters.
  • missing err check when calling cache.Put

See b8fea7d

Tell me if it's OK for you, if yes I will approve the PR 👍

I also made the cache functions private 66e9463

@joshLong145
Copy link
Contributor Author

@joshLong145 I refactored a little bit the cache part :

  • use .ignite/plugins as root dir
  • remove usage of intermediary ConfigContext (the tests are passing, so I assume I solved the problem you had with the net.Addr, but maybe you should check again please)
  • ReadPluginConfCache returns a ReattachConfig instead of filling the one in paramters.
  • missing err check when calling cache.Put

See b8fea7d

Tell me if it's OK for you, if yes I will approve the PR 👍

I also made the cache functions private 66e9463

@tbruyelle
These changes look good to me. Verified they don't regress functionality with the changes to the plugin cache.

@joshLong145 joshLong145 requested a review from tbruyelle January 1, 2023 22:10
Copy link
Contributor

@tbruyelle tbruyelle left a comment

Choose a reason for hiding this comment

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

Thanks for this feature @joshLong145 , will be useful for sure !

@aljo242 aljo242 merged commit 015b140 into main Jan 3, 2023
@aljo242 aljo242 deleted the feat/plugin-host branch January 3, 2023 14:12
Jchicode pushed a commit to Jchicode/cli that referenced this pull request Aug 9, 2023
* start to plugin host impl wip

* comments

* minor bug updates

* updates per plugin config migration

* small updates to sharedHost plugin option

* update to isolate plugin cache

* rename of utils to cache

* print line removing

* tidy

* removing print out

* additional comments

* shared host load test

* addition of caching tests for plugins

* fmt

* changelog

* additional plugin cache tests

* update shared host tests

* review comments

* addition cache test cases

* fix typo

* fix comment

* update plugin `KillClient` for shared hosts

* changelog update

* fix test

by omitting SharedHost from the yml when its false

* update: migration of `sharedHost` flag to plugin manifest

* scaffolding and plugin host check changes

* update tests

* format and lint

* update plugin cache test

* update property comment

* update plugin cache to use full plugin path

* cleanup of plugin tests

* lint

* update plugin sharedHost tests

* update to plugin docs for `Sharedhost` flag

* test: fix TestPluginLoadSharedHost

* refac: plugin cach

* Update docs/docs/contributing/01-plugins.md

* refac: plugin cache func should be private

* test: improve plugin kill assertions

Co-authored-by: Thomas Bruyelle <[email protected]>
Co-authored-by: Thomas Bruyelle <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:extensions Related to Ignite Extensions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants