[asterisk-dev] [Code Review] 3281: stasis cache: Enhance to keep track of an item from different entities.

Mon Mar 3 17:45:45 CST 2014

> On March 2, 2014, 11:01 p.m., Matt Jordan wrote:
> > /trunk/include/asterisk/stasis.h, lines 735-746
> > <https://reviewboard.asterisk.org/r/3281/diff/1/?file=55015#file55015line735>
> >
> >     I might use different terms other than internal/external. I know that's probably what we used when we discussed it, but reading through the API it might get confusing - something being 'internal' might relate to a private portion of the cache, or something similar.
> >     
> >     Maybe local/remote?
> >     
> >     stasis_cache_entry_get_local(struct stasis_cache_entry *entry);
> >     
> >     stasis_cache_entry_get_remote(...);

Local/remote does work better than internal/external so I'll change it.

> On March 2, 2014, 11:01 p.m., Matt Jordan wrote:
> > /trunk/include/asterisk/devicestate.h, lines 278-281
> > <https://reviewboard.asterisk.org/r/3281/diff/1/?file=55014#file55014line278>
> >
> >     I'm certainly fine with losing the cache_id, as it is no longer necessary.
> >     
> >     I'm not fan of changing the stringfield to a const char *, simply because it really is non-const, and you have to cast the const char * back to a char * to free it. Generally, if something is const it should always be const; if not, well, it's not.
> >     
> >     Granted, string fields with a single entry a bit goofy, but I think I'd rather go with that goofiness over the const gyrations.

The device name string is immutable throughout its lifetime.  It is just malloced by ast_strdup() to hold the value.  It is a quirk of the free() function signature that it takes a non-const pointer and thus requires the cast to get rid of the memory.

Stringfield strings are declared as const when they are used.  You have to call a function to set a new value.

typedef const char * ast_string_field;
#define AST_STRING_FIELD(name) const ast_string_field name

> On March 2, 2014, 11:01 p.m., Matt Jordan wrote:
> > /trunk/main/devicestate.c, lines 656-661
> > <https://reviewboard.asterisk.org/r/3281/diff/1/?file=55017#file55017line656>
> >
> >     I don't entirely understand the purpose of this callback.
> >     
> >     We are re-publishing non-cacheable messages that are either from ourselves or from others. This is actually from a caching topic, which makes me wonder how we got a non-cacheable message in the first place.

The purpose of this function is to create the "aggregate" state for non-cacheable device states.  Most subscribers only care about the aggregate device state and ignore the other device state messages.

This seems to be a minor consistency bug from before.  The cache subscribes to ast_device_state_topic_all() so it gets all device state events (cacheable and non-cacheable).  The cache subscription does not filter out non-cached events so it shouldn't make any difference in this case.

I have fixed the inconsistency.

> On March 2, 2014, 11:01 p.m., Matt Jordan wrote:
> > /trunk/main/stasis_cache.c, line 132
> > <https://reviewboard.asterisk.org/r/3281/diff/1/?file=55018#file55018line132>
> >
> >     For the same reason previously, this should just be kept a char *.
> >     
> >     In particular, this struct is internal to stasis_cache. The risk of someone inadvertently treating it as a mutable object is low, and it technically is mutable.
> >     
> >     Even if you shouldn't change it.

For a similar reason as before it should be const.  It is a quirk of the free() signature to require the cast.
It is immutable and it is used as the key to the cache entries container.  Changing the container key corrupts the container if the item is still in the container.

- rmudgett

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3281/#review11018
-----------------------------------------------------------

On March 1, 2014, 7:07 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3281/
> -----------------------------------------------------------
> 
> (Updated March 1, 2014, 7:07 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23204
>     https://issues.asterisk.org/jira/browse/ASTERISK-23204
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> A stasis cache entry now contains more than a single message/snapshot.  It contains messages/snapshots for the internal entity as well as any external entities that post to the cached item.  In addition callbacks can be supplied when the cache is created to compute and post the aggregate message/snapshot representing all entities stored in the cache entry.
> 
> * All stasis messages now have an eid to indicate what entity posted it.
> 
> * The stasis cache enhancements allow device state to cache and aggregate the device states from internal and external entities in a single operation.  The cached aggregate device state is available immediately after it is posted to the stasis bus.  This improves performance by eliminating a cache dump and associated ao2 container traversals to calculate the aggregate state.
> 
> 
> Diffs
> -----
> 
>   /trunk/tests/test_devicestate.c 409306 
>   /trunk/main/stasis_message.c 409306 
>   /trunk/main/stasis_cache.c 409306 
>   /trunk/main/devicestate.c 409306 
>   /trunk/main/app.c 409306 
>   /trunk/include/asterisk/stasis.h 409306 
>   /trunk/include/asterisk/devicestate.h 409306 
> 
> Diff: https://reviewboard.asterisk.org/r/3281/diff/
> 
> 
> Testing
> -------
> 
> All unit tests pass including the stasis and device state tests.
> 
> The device state unit test had to be changed to get the aggregate state out of the new cache location.  Fortunately, the normal users of the device state aggregate information subscribe to the events and don't get the aggregate device state out of the cache.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140303/79fad4bd/attachment-0001.html>