----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3305/#review11078 -----------------------------------------------------------
Another thing I neglected to mention in my review that concerns me is the performance hit this patch could cause. Currently, the object fields on a sorcery object type are an ao2_container that is a linked list. The algorithm for filtering out unneeded object fields is of O(M*N) where M is the number of retrieved fields from the database and N is the number of object fields registered for the object type. If the object fields were changed to use a different container type then the complexity could be reduced to O(M*log(n)) if using a red-black tree or O(M) if using a hash table (assuming no hash collisions). I did not make a change to the container type as I was not sure if the ordering of the objects was important in the container. If so, then the red-black tree option could be used with some small modifications to the ast_sorcery_object_field structure. - Mark Michelson On March 5, 2014, 11:40 p.m., Mark Michelson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3305/ > ----------------------------------------------------------- > > (Updated March 5, 2014, 11:40 p.m.) > > > Review request for Asterisk Developers and Joshua Colp. > > > Repository: Asterisk > > > Description > ------- > > When testing out realtime for PJSIP, I encountered a problem. In my database, > I had defined additional columns beyond the ones that Asterisk cared about. > As a result, when data was pulled from the database, the config framework > barfed, saying it didn't know what these object fields were. For config > files, this practice is fantastic. It makes sure to raise a giant red flag to > people that they are attempting to configure something that is not recognized > by Asterisk. For realtime, on the other hand, it is legitimate for a database > to contain supplemental data alongside the data Asterisk cares about. > > What I have done is to modify res_sorcery_realtime.c to filter out fields > from the retrieved objectset that have not been registered with sorcery. In > addition, I have created two realtime sorcery unit tests. The first one > simply creates data in a realtime backend and then has sorcery atttempt to > retrieve the data, ensuring that an object is allocated with the expected > data. The second test is similar, except that in addition to the data that > has been registered with sorcery, an additional field is created in the > realtime backend. This test ensures that the data can still be retrieved and > successfully allocated by sorcery. > > Of all things in this review, the thing I'm concerned about most is the log > message that has been added to indicate that an object has been filtered out > of the resultset retrieved from realtime. I initally wanted to make this a > NOTICE-level message, but I was afraid that with the frequency with which > objects are retrieved from realtime, this could spam the logs a little too > much. On the other hand, leaving it as a level 1 debug message I have now > could make it difficult for someone that has misconfigured their database > (say they accidentally have a typo in a column name) from seeing that data > they expect to be relevant is being ignored by Asterisk. Any suggestions on > how to handle this? > > > Diffs > ----- > > /branches/12/tests/test_sorcery_realtime.c 409886 > /branches/12/res/res_sorcery_realtime.c 409886 > /branches/12/main/sorcery.c 409886 > /branches/12/include/asterisk/sorcery.h 409886 > > Diff: https://reviewboard.asterisk.org/r/3305/diff/ > > > Testing > ------- > > All realtime sorcery unit tests pass with flying colors. > > > Thanks, > > Mark Michelson > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev