[TSDR-88] Refactor/Simplify SyslogDatastoreManager Created: 19/Sep/18 Updated: 03/Oct/18 Resolved: 20/Sep/18 |
|
| Status: | Resolved |
| Project: | tsdr |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Medium |
| Reporter: | Tom Pantelis | Assignee: | Tom Pantelis |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Description |
|
The SyslogDatastoreManager implements a mechanism for an external client to register a callback URL to be notified of syslog messages that pass a specified filter. Registration is done via a register-filter RPC. It generates a filter ID and writes a SyslogFilter list entry to the CONFIGURATION store. It also generates a listener ID and writes a Listener list entry under the SyslogFilter entry. A RegisteredListener instance is instantiated to listen for updates to the SyslogListener list entry in the OPERATIONAL store. When a syslog message is raised, if it passes the filter, the message text is written to the SyslogListener entry where, upon change notification, the RegisteredListener sends it to the callback URL. I don't know the original use case for this functionality and why it was added. Unfortunately the patch that added it, https://git.opendaylight.org/gerrit/#/c/41711/20, does not explain the reason. A few things I observed:
Assuming we want to keep this functionality, I propose we simplify it and make it work properly across restarts. Remove the listener lists from the yang model and simplify to: container syslog-dispatcher {
list syslog-filter {
config true;
key "filter-id";
leaf filter-id {
type string;
description "the unique ID of registered filter.";
}
container filter {
uses meta-filter;
}
leaf callback-url {
type string;
description "callback URL of your app.";
}
}
Eliminate the register and delete filter RPCs. Users can write syslog-filter entries to register. Add a DTCL for the syslog-filter list that caches the entries for quick retrieval. When a syslog message is raised, queue a thread task fo each registered filter to offload the notification. |
| Comments |
| Comment by Tom Pantelis [ 19/Sep/18 ] |
| Comment by Scott Melton [ 03/Oct/18 ] |
|
I don't recall the use case for an external client to register a callback URL either. As you mention the code supports only one listener entry. I'd guess the listener list was implemented for future development and can be simplified to contain only one, at least until we have a use case and modify the code to support it. Yes, I wouldn't be happy about using the OPERATIONAL datastore either. We should conform to how the different data stores are intended to be used. The implementors were probably unaware of that. Yes, the listener registration should be persisted. It doesn't make sense not to. I'd bet it was needed just never implemented. That syslog-dispatcher container looks good, but I'm not that familiar with it. Would adding a "the" to "the unique ID of "the" registered filter." and spelling out application instead of the abbreviation app make them more readable? Maybe it's just me, but I think it is old school to shorten a DSL naming convention or descriptions when it isn't needed. I'm finding the more complete a DSL the easier it is to learn. It also looks better to the untrained eye. "app" is a reasonable abbreviation in that context and wouldn't be confused with another word, but others can be ambiguous. Abbreviations also make the mind take extra step to guess or question at the intended word. |
| Comment by Tom Pantelis [ 03/Oct/18 ] |
|
I assume you referring to the yang descriptions. I didn't change any of those in my patch but I'm sure they can be improved. Would you like to push a patch? |
| Comment by Scott Melton [ 03/Oct/18 ] |
|
Ok, thought that was new. I would like to push many of those kinds of patches. Maybe I can get to the code cleanup, TSDR-68 soon. |