[Asterisk-code-review] res sorcery memory cache: Add support for a full backend cache. (asterisk[13])

[Kevin Harwell]

Kevin Harwell has posted comments on this change.

Change subject: res_sorcery_memory_cache: Add support for a full backend cache.
......................................................................


Patch Set 4:

(4 comments)

https://gerrit.asterisk.org/#/c/1808/4/res/res_sorcery_memory_cache.c
File res/res_sorcery_memory_cache.c:

Line 250: static void start_passthru_update(void)
        : {
        : 	uint32_t *passthru_update_thread_id;
        : 
        : 	passthru_update_thread_id = ast_threadstorage_get(&passthru_update_id_storage,
        : 		sizeof(*passthru_update_thread_id));
        : 	if (!passthru_update_thread_id) {
        : 		ast_log(LOG_ERROR, "Could not set passthru update ID for sorcery memory cache thread\n");
        : 		return;
        : 	}
        : 
        : 	*passthru_update_thread_id = PASSTHRU_UPDATE_THREAD_ID;
        : }
        : 
        : static void end_passthru_update(void)
        : {
        : 	uint32_t *passthru_update_thread_id;
        : 
        : 	passthru_update_thread_id = ast_threadstorage_get(&passthru_update_id_storage,
        : 		sizeof(*passthru_update_thread_id));
        : 	if (!passthru_update_thread_id) {
        : 		ast_log(LOG_ERROR, "Could not set passthru update ID for sorcery memory cache thread\n");
        : 		return;
        : 	}
        : 
        : 	*passthru_update_thread_id = 0;
        : }
These two functions are almost identical. Could they be combined into one?


Line 851: 
        : 	cached = sorcery_memory_cached_object_alloc(arg, cache, obj);
        : 	if (!cached) {
        : 		return 0;
        : 	}
Should this return a CMP_STOP in order to stop processing since a failure occurred?


Line 936: 		remove_all_from_cache(task_data->cache);
Should this be locked as well when removing?


Line 1027: 	struct ao2_container *backend_objects;
         : 
         : 	start_passthru_update();
         : 	backend_objects = ast_sorcery_retrieve_by_fields(sorcery, type, AST_RETRIEVE_FLAG_MULTIPLE | AST_RETRIEVE_FLAG_ALL, NULL);
         : 	end_passthru_update();
         : 
         : 	if (!backend_objects) {
         : 		/* This will occur in off-nominal memory allocation failure scenarios */
         : 		return;
         : 	}
         : 
         : 	if (cache->maximum_objects && ao2_container_count(backend_objects) >= cache->maximum_objects) {
         : 		ast_log(LOG_ERROR, "The backend contains %d objects while the sorcery memory cache '%s' is explicitly configured to only allow %d\n",
         : 			ao2_container_count(backend_objects), cache->name, cache->maximum_objects);
         : 		return;
         : 	}
         : 
         : 	ao2_callback_data(backend_objects, OBJ_NOLOCK | OBJ_NODATA | OBJ_MULTIPLE, object_add_to_cache_callback,
         : 		(struct ast_sorcery*)sorcery, cache);
         : 
         : 	/* If the number of cached objects does not match the number of backend objects we encountered a memory allocation
         : 	 * failure and the cache is incomplete, so drop everything and fall back to querying the backend directly
         : 	 * as it may be able to provide what is wanted.
         : 	 */
         : 	if (ao2_container_count(cache->objects) != ao2_container_count(backend_objects)) {
         : 		ast_log(LOG_WARNING, "The backend contains %d objects while only %d could be added to sorcery memory cache '%s'\n",
         : 			ao2_container_count(backend_objects), ao2_container_count(cache->objects), cache->name);
         : 		remove_all_from_cache(cache);
         : 		abort();
         : 	}
         : 
         : 	ao2_ref(backend_objects, -1);
Some duped/similar code to the stale_cache_update function. Think it would be worthwhile abstracting out the shared parts or can that function be made to call this one?


-- 
To view, visit https://gerrit.asterisk.org/1808
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie2993487e9c19de563413ad5561c7403b48caab5
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-HasComments: Yes