zabbix-tools / go-zabbix

Go bindings for the Zabbix API
GNU General Public License v2.0
108 stars 70 forks source link

[Feature] Session caching #10

Closed x1unix closed 6 years ago

x1unix commented 6 years ago

Changelog

Checks

API

Unit tests

Example

cache := zabbix.NewSessionFileCache().SetFilePath("./zabbix_session")
session, err := zabbix.CreateClient("http://zabbix/api_jsonrpc.php").
    WithCache(cache).
    WithCredentials("Admin", "zabbix").
    Connect()

fmt.Printf("Connected to Zabbix API v%s", session.Version())

SessionBuilder.Connect() will check the cache for cached sessions. If a valid cached session exists - it will provide it, otherwize will connect to Zabbix and will save a session (if cache was provided).

x1unix commented 6 years ago

@cavaliercoder, thank you for adding me as a repo collaborator!

Can you review this PR, please? This PR is huge and introduces some new API's and approaches.

cavaliercoder commented 6 years ago

You're welcome! Your last few PRs have been really helpful, and I am so slow at responding. Hopefully the community will get value from you being able to fix things faster.

cavaliercoder commented 6 years ago

Thanks for putting the time into this, and running it by me. I'm pretty happy with the session caching logic, but I think the API could maybe be made a little simpler. There are quite a few moving parts for the developer to understand.

Could you achieve the same outcome with two functions, Session.Save(cachePath) and RestoreSession(cachePath, maxAge). So, for the caller:

session, err := zabbix.RestoreSession(cachePath, maxAge)
if err == zabbix.ErrNoSession { // file not found or expired, etc.
    session, err = zabbix.NewSession(.......).WithCache(cachePath)
}
if err != nil {
    // handle
}

WithCache could simple wrap Session.Save and return the underlying session.

I'm not married to the above code - just an example - but I'm keen to avoid introducing new types that might not be needed, like SessionFileCache, SessionAbstractCache, ClientBuilder, etc.

I'd also recommend decoupling file modified date from the session start date. There are other reasons the file date could change and it could break your cache. You could store the session date in the file?

Great feature!

x1unix commented 6 years ago

@cavaliercoder, thank you for the feedback. I've introduced an external session builder to single responsibility principle and to provide a simpler API for custom caching.

It might have some logic overhead (I was inspired by Java approach 😄 ) and might be improved in future.

The default file system cache introduced more as a PoC demo than a production solution, but thank you for the feedback again!

cavaliercoder commented 6 years ago

I suspected .Net actually, but Java has very similar idioms! 🤣Go tends to take a more minimalist approach, with all the associated tradeoffs. Thanks for approaching this so thoughtfully.