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

[Matt Jordan]

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



/trunk/include/asterisk/devicestate.h
<https://reviewboard.asterisk.org/r/3281/#comment20649>

    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.



/trunk/include/asterisk/stasis.h
<https://reviewboard.asterisk.org/r/3281/#comment20654>

    Both of these need corresponding unit tests in test_stasis



/trunk/include/asterisk/stasis.h
<https://reviewboard.asterisk.org/r/3281/#comment20650>

    I think it might be worth putting a \note or something similar and explaining what an aggregate message is.
    
    It's a relatively odd concept, and may not be perfectly obvious to someone browsing the API.



/trunk/include/asterisk/stasis.h
<https://reviewboard.asterisk.org/r/3281/#comment20655>

    Unit tests for all!
    
    Stasis lends itself incredibly well to being unit tested. We should avail ourselves of that. I'd expect unit tests for creating a cache the supports the new callbacks, tests that the callbacks are called at the appropriate times, and that internal/external/aggregate items can be retrieved from the cache after being put into it.



/trunk/include/asterisk/stasis.h
<https://reviewboard.asterisk.org/r/3281/#comment20651>

    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(...);



/trunk/main/devicestate.c
<https://reviewboard.asterisk.org/r/3281/#comment20652>

    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.



/trunk/main/stasis_cache.c
<https://reviewboard.asterisk.org/r/3281/#comment20653>

    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.


- Matt Jordan


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/9b6f5d9f/attachment-0001.html>