The acquia_search_enable_acquia_solr_environment() function is responsible for maintaining the environment connected to Acquia Search and is called during cron runs and when the Acquia Search module is enabled. The problem is that this function is called twice when the module is enabled and three times during successful cron runs. Although I cannot confirm, this may be the reason why people are getting PDO exceptions when installing Acquia Search due to an integrity constraint violation. See #1778050: Stale cache when CTools is already enabled. for more details.

To give an overview, acquia_agent_cron() function checks the subscription, which invokes hook_acquia_subscription_status(). Acquia Search implements this hook and calls acquia_search_enable_acquia_solr_environment(). So far, so good.

The root problem is that acquia_search_enable() explicitly calls acquia_search_enable_acquia_solr_environment() and then sends another heartbeat via acquia_agent_check_subscription(), which invokes hook_acquia_subscription_status() thus calling acquia_search_enable_acquia_solr_environment() again. An extra check happens on cron because acquia_search_cron() calls acquia_search_enable().

CommentFileSizeAuthor
#15 1784770-15.patch2.74 KBnick_vh
#14 1784770-14.patch2.74 KBnick_vh
#9 1784770-9.patch2.2 KBpwolanin
#2 1784770-2.patch776 bytesnick_vh
#1 multiple-environment-saves-1784770-1.patch438 bytescpliakas

Comments

cpliakas’s picture

cpliakas commented
Status: Active » Needs review
StatusFileSize
new multiple-environment-saves-1784770-1.patch438 bytes

As a quick fix, removing the call to acquia_search_enable() in acquia_search_cron() prevents this function from being called three times during cron. It is still called twice when the module is enabled, however that is probably a separate issue that can be resolved independently of this one.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
StatusFileSize
new 1784770-2.patch776 bytes

We could remove two checks

acquia_search_enable() calls acquia_agent_check_subscription, that calls acquia_search_acquia_subscription_status and, if the sub is active, it will execute acquia_search_enable_acquia_solr_environment.

Thanks for starting this one!

cpliakas’s picture

cpliakas commented
Status: Needs review » Needs work

Are we sure about the consequences of removing acquia_search_enable_acquia_solr_environment() in acquia_search_enabled()? I explicitly left that out because I am pretty sure (but not positive) there is a reason for this. Specifically, the acquia_search_enable_acquia_solr_environment() reacts after the subscription is checked. I think that the heartbeat needs to be sent again so that the refreshed Acquia Search data can be sent in acquia_agent_get_subscription().

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

acquia_search_enable() calls acquia_agent_subscription_is_active
If acquia_agent_subscription_is_active() is true execute acquia_agent_check_subscription()
acquia_agent_check_subscription tries to define if the sub is active using acquia_agent_subscription_is_active() (which he actually already verified...)
acquia_agent_check_subscription does not do anything with the search environment afaik, so I don't see a dependency here.
acquia_agent_check_subscription calls module_invoke_all('acquia_subscription_status', $active, $subscription);
acquia_search_acquia_subscription_status() is invoked and if $active is TRUE, it executes acquia_search_enable_acquia_solr_environment();

So this is the exact same logic for the previous one
acquia_search_enable() calls acquia_agent_subscription_is_active()
If acquia_agent_subscription_is_active() is true, execute acquia_search_enable_acquia_solr_environment();

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Status: Needs work » Needs review
cpliakas’s picture

cpliakas commented
Status: Needs review » Reviewed & tested by the community

OK. Makes sense, and thanks for the detailed explanation. I also ran this through a debugger just to make sure the first heartbeat had all of the relevant information, and it appears to pass along the same data with and without this patch. Marking as RTBC, would love to get this triple checked if possible by someone who has the context as to why this was required once upon a time.

coltrane’s picture

coltrane
he/him
commented

I think this makes sense. I added a watchdog call to acquia_search_enable_acquia_solr_environment() and its called once when Acquia Search is enabled and once during a cron run with patch in #2 applied.

I'd like to have final sound off from Peter tho before this gets committed.

cpliakas’s picture

cpliakas commented

+1. Thanks for reviewing!

pwolanin’s picture

pwolanin commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 1784770-9.patch2.2 KB

I think we can do away with the check in hook_enable, but need more code comments to explain the flow.

Also, we should set the version before that for improved accuracy.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

So in short

  • Send the version if the module is updated or if the requirement are checked on the status page
  • When we enable the module we also send the version & we call acquia_agent_check_subscription
  • acquia_agent_check_subscription tries to define if the sub is active using acquia_agent_subscription_is_active()
  • acquia_agent_check_subscription calls module_invoke_all('acquia_subscription_status', $active, $subscription);
  • acquia_search_acquia_subscription_status() is invoked and if $active is TRUE, it executes acquia_search_enable_acquia_solr_environment();

acquia_search_cron dies here also.

The patch of Peter gets rid of the double is_active check on top of what was removed already.

This looks good to me, will test it in a complete flow now

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Project: Acquia Connector » Acquia Search
Version: 7.x-2.x-dev » 6.x-3.x-dev
Status: Needs review » Patch (to be ported)

Committed to 7.x-2.x. moving this to Acquia Search for a backport to 6.x-3.x

cpliakas’s picture

cpliakas commented
Project: Acquia Search » Acquia Connector
Version: 6.x-3.x-dev » 7.x-2.x-dev
Status: Patch (to be ported) » Needs review

Patch looks good and eliminates the double check. The one thing I am confused about is the code below:

+  // Update the cached version whenever we may be updating the module.
+  if ($phase == 'runtime' || $phase == 'update') {
+    _acquia_search_set_version();
+  }

Why are we doing this check in the runtime phase as well? It isn't a huge performance hit, but it doesn't seem right to me for this code to run everytime someone visits the /admin/config page.

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Project: Acquia Connector » Acquia Search
Version: 7.x-2.x-dev » 6.x-3.x-dev
Status: Needs review » Patch (to be ported)

As discussed over IM, this is due to the fact that an updated version can happen without the update.php or the acquia_search_enable() function have ran.

We need a place so we can say with some certainty that the version that is being sent with the search requests is the correct version. However, this has nothing to do with the info we send over RPC to our service. I'd say we can let it rest in piece because this is more for statistical information.

Alternatives might this ever become a problem :
Remove the runtime from the requirements
add _acquia_search_set_version() to acquia_search_acquia_subscription_status() as this is run during a cron loop.

moving it back to Acquia Search now

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 1784770-14.patch2.74 KB

backporting to 6.x-3.x is a little difficult since we do not have the update phase (there is one, but only for the system module.)

I added the version set to the cron and added explanations where possible

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
StatusFileSize
new 1784770-15.patch2.74 KB
nick_vh’s picture

nick_vh
he/him
Location Ghent
commented

Committed to 6.x-3.x after #1778282: Apache Solr Search Integration environment deleted when rpc.acquia.com is inaccessible got committed. No need to backport any of this to 6.x-1.x

nick_vh’s picture

nick_vh
he/him
Location Ghent
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.