> On March 5, 2014, 5:50 p.m., Mark Michelson wrote: > > 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.
I don't any reason that the object fields container order is important. You likely would want to use a hash container with 17 or so buckets. There shouldn't be too many object fields. The container is going to be searched more than anything and doesn't need to be sorted. As for the spaming message, I don't think you have much choice but to use the ast_debug() version. Any NOTICE level message would be spam if the column is intentionally there. The only remaining option is to create a new option to have Asterisk complain about questionable items unless one already exists. - rmudgett ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3305/#review11078 ----------------------------------------------------------- On March 5, 2014, 5: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, 5: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
