Domain conf - bad SQL perfrmance

Updated title.
Implemented the CRUD utilities mentioned in the solution proposal.
Added one more for deletion.
Refactored the code in the module to utilize the new code-base, it shrank substantially (at least in line length).

Uploading patch.
TODO: Add the cache logic on top of it.

Renamed the CRUD utilities to follow the same namespace: domain_conf_data_(get|set|delete).
Added static and persistent cache.
Attaching patch and diff to the previous one from #2.

Fixed a small issue in the cache invalidation logic in the delete.
Ignore patch in #3.

- Added new utility parameter to the set method called $merge, defaults to TRUE. When TRUE, it will merge the new settings on top of the old ones. When FALSE, it will overwrite the whole settings array with the new value.
- Improved docks.
- Refactoring and fixes to utilize correctly the new $merge parameter and the previously introduced $reset in the get utility.
- Uploading a new patch and diff to the previous one.

Updated description with more information.

Fix typos, causing notices... Uploading patch and diff to previous.

TODOs I came up with for the last week.
- The use if the 3 new methods needs to be checked thoroughly again for the need to proxy some reset calls (maybe).
- The persistent cache should be moved to cache_bootstrap, instead of the default cache bin. This is because variables API in core is using it and data we will be holding there is pretty much the same kind (configuration). This is expected to improve bootstrap speeds when used with some advanced cache configurations even more. Ones like this.

So here is the new patch, apparently there were issues...
1. After the re-audit I've found 1 place, where a reset flag was not passed correctly.
2. Moved the cache bin the cache_bootstrap.

Uploading patch and diff to #8. Hiding old ones.

Forget to change the status.

None at the moment...

Do we have any form of test coverage on the old functions, as this is just data acquisition low level layer. It replaces a DB query. So if the old tests pass this will not cause regressions. If there are no old tests, I can not see the scope of this issue to cover that. How should we proceed with that?

It will not be trivial to cover everything, but that's for a separate issue anyway.

As there are 3 new functions I agree that they need to be covered, so I am marking as needs work - to add the testing for the new API.

After reviewing the code again I think that:
1. domain_conf_features_rebuild this can be further optimized to make only 1 write query (delete or update), not up-to 2 as it is now (delete and maybe insert).

2. The domain_conf_data_delete should force (int) cast to the domain_id passed to the DB. Maybe add input param validation, as we have int and 'all' and currently we trust the developers that the method will be used correctly.

Hi, here are the tests against this new API.

During the testing, there seems to be an issue with either memcache (in my local set-up - not likely, as it a way too simple to configure...) or this functionality in general against any custom cache-api back-end. With my settings.php like so:

/**
 * Add the domain module setup routine.
 */
include DRUPAL_ROOT . '/sites/all/modules/domain/settings.inc';

// Memcache...
$conf['cache_backends'][] = 'sites/all/modules/memcache/memcache.inc';
$conf['memcache_servers'] = array('127.0.0.1:11211' => 'default');
$conf['memcache_key_prefix'] = $databases['default']['default']['database'];
$conf['cache_default_class'] = 'MemCacheDrupal';
$conf['cache_class_cache_form'] = 'DrupalDatabaseCache';

All the new tests pass. With the settings configured like this:

// Memcache...
$conf['cache_backends'][] = 'sites/all/modules/memcache/memcache.inc';
$conf['memcache_servers'] = array('127.0.0.1:11211' => 'default');
$conf['memcache_key_prefix'] = $databases['default']['default']['database'];
$conf['cache_default_class'] = 'MemCacheDrupal';
$conf['cache_class_cache_form'] = 'DrupalDatabaseCache';

/**
 * Add the domain module setup routine.
 */
include DRUPAL_ROOT . '/sites/all/modules/domain/settings.inc';

They don't due to persistent cache not working (cache-miss during the tests)...

It seems that it works with DB as cache back-end (as the custom one is not configured yet), for alternative cache back-ends it might be either too-early in the bootstrap or not well thought out (or my local set-up issue)...

Too strange to debug in the evening :).

Fast workaround would be to have static cache only, but this will negate most of the positives from this patch, as DB will still be accessed for this type of data. Still code structure will be greatly improved. Any feedback and also testing at your end with / without memcache would be greatly appreciated :)

Notes 1 & 2 from #15 are still pending, as I was stuck on the described issue with the cache_api.

I will be able to look into this next Sunday (maybe) if there is no progresses.

#19 The best way to test for back-end performance is for time to first byte with AB or even better - XHProf as it will give more insight. Note that page cache should be disabled. In #17 I was referring to testing in the way the patch works with memcache at all or not, as I had issues as already described. Apparently not with the memcache instance :( .

#20 I think we need to fix the issue with the overriding (if it's an issue). If just a docks change is OK then.

But... The issue here was the need to make the query to hit a cache layer and not the DB. As it is, with the working scenario, the domain_boostrap(); will utilize DB for extracting the data and then we will have memcache configured.

In the docks at the moment there is clearly stated that usually the line should be at the end:

IMPORTANT: You must add these lines to settings.php after the
$databases. Normally, you should add these lines to the end of the
settings.php file.

So it is expected to have it as the currently broken case (configs at the end).
@agentrickard Can you confirm this, or this will need more debugging :D

Other way to make this moving and have some progress is to out-scope the persistent cache into a follow-up, as it's a clearly a separate issue that is more complex.

Here is the new patch with persistent cache layer out if it + a partial diff.
The new tests now pass always.
The notes from #15 are fixed.
I've opened the issue that we've discussed and agreed in #22 and #23 as postponed on this one.
I've hidden all the patches except the latest ones from the issue description.
Updated issue summary - proposal section to note the new child issue.

@agentrickard, did you have any time to test this?