geyser: allow custom name in config file

[fanatid]

Problem

Not possible to use the same plugin twice. If we want to use the same plugin with different configs we receive an error, for example rpcpool/solana-accountsdb-plugin-kafka#23

Summary of Changes

Allow optional libname field in plugin config.

cc @mohsen-vybenetwork @Lusitaniae

Q: do we have some docs in the repo which should be changed?
Q2: should we put Library into LoadedGeyserPlugin? I think it should be easier to manage loading/unloading

When PR will be merged I'll work on backport to 1.16

[lijunwangs]

Maybe libname? I do not see how this utilized to solve the problem. We intentionally request the plugin has different names so that load/unload can work reliably. Why cannot you give different names to different plugins?

[fanatid]

Why cannot you give different names to different plugins?

because for a new name you need to re-compile the whole plugin with hardcoded name in the code 🙂

solana/geyser-plugin-interface/src/geyser_plugin_interface.rs

Line 290 in 666ce9b

fn name(&self) -> &'static str;

[lijunwangs]

I think this problem can be solved by the plugin itself without the framework's involvement. The &static reference makes it a little tricky but not insurmountable. Here is some sample code to the effect:

use std::fs;
use std::sync::{Mutex, Once};

use std::collections::HashMap;

struct Plugin {
    name: Mutex<HashMap<i32, &'static str>>,
    initialized: Once,
}

impl Plugin {
    pub fn new() -> Self {
        Self {
            name: Mutex::new(HashMap::default()),
            initialized: Once::new(),
        }
    }

    pub fn load(&mut self, file: String) {
        self.initialized.call_once(|| {
            let file_content: String = fs::read_to_string(file).unwrap();
            let s = Box::new(file_content);
            let s = Box::leak(s); // it leaks once and only once, so still safe!
            self.name.lock().unwrap().insert(1, s);
        })
    }

    fn name(&self) -> &'static str {
        let data = self.name.lock().unwrap();
        let data = data.get(&1).unwrap();
        data
    }
}

fn main() {
    let mut s = Plugin::new();
    s.load("config.txt".to_string());
    println!("This is my content {}", s.name());
}

This way, you don't have to hard code the plugin name in the code and load it from the config file yourself.

[fanatid]

haha, nice workaround
I think name() happens before load() so call_once should be moved in this case

while this code probably would work I still think that supporting libname through config would be nice
for plugins users, it should be much easier to add one line to config instead of patching the plugin with the proposed code

[fanatid]

yep, confirmed
on_load is called after name, so we can not take the name from the config file in the plugin

solana/geyser-plugin-manager/src/geyser_plugin_manager.rs

Lines 95 to 113 in 64b3613

	if self
	.plugins
	.iter()
	.any(|plugin| plugin.name().eq(new_plugin.name()))
	{
	return Err(jsonrpc_core::Error {
	code: ErrorCode::InvalidRequest,
	message: format!(
	"There already exists a plugin named {} loaded. Did not load requested plugin",
	new_plugin.name()
	),
	data: None,
	});
	}
	// Call on_load and push plugin
	new_plugin
	.on_load(new_config_file)
	.map_err(|on_load_err| jsonrpc_core::Error {

[lijunwangs]

I think we should make it optional now. If it is set in the config file, use it, otherwise old behavior to keep it backward compatible. I would like to still call it "name" instead of libname, we do not care about the libname. The lib path has the libname anyway.

[fanatid]

Changed libname to name. Value is already optional.

[fanatid]

@lijunwangs ping :)

[codecov]

Codecov Report

Merging #33550 (d9932ea) into master (8fbe033) will increase coverage by 0.0%.
Report is 13 commits behind head on master.
The diff coverage is 58.3%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #33550   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         819      819           
  Lines      220084   220103   +19     
=======================================
+ Hits       180347   180420   +73     
+ Misses      39737    39683   -54     

[lijunwangs]

Looks good! Thanks

[lijunwangs]

@fanatid Looks like there are some merge issues.

[lijunwangs]

Need to address merge issues

[fanatid]

Updated

[lijunwangs]

Looks good! Thank you

[mergify]

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.