Added option to select replica type per attribute #1421
Conversation
There was a problem hiding this comment.
@rachel-trott @mohitalgolia Please see my comments with regards to release 3.13.2
I think this feature needs revisiting prior to release.
| @@ -143,6 +143,7 @@ class ConfigHelper | |||
| public const ENHANCED_QUEUE_ARCHIVE = 'algoliasearch_advanced/queue/enhanced_archive'; | |||
| public const NUMBER_OF_ELEMENT_BY_PAGE = 'algoliasearch_advanced/queue/number_of_element_by_page'; | |||
| public const ARCHIVE_LOG_CLEAR_LIMIT = 'algoliasearch_advanced/queue/archive_clear_limit'; | |||
| public const MAX_VIRTUAL_REPLICA_COUNT = 50; | |||
There was a problem hiding this comment.
| if ($virtualReplicaCount < self::MAX_VIRTUAL_REPLICA_COUNT && ($defaultVirtualReplicaEnabled || (isset($attr['virtualReplica']) && $attr['virtualReplica']))){ | ||
| $virtualReplica = 1; | ||
| } else { | ||
| $virtualReplica = 0; |
There was a problem hiding this comment.
We should be communicating an error here through the message manager not silently burying exceeding the limit. Something like a ReplicaLimitExceededException should be thrown and caught for rendering in the UI.
| @@ -1028,6 +1036,7 @@ public function getSortingIndices($originalIndexName, $storeId = null, $currentC | |||
| $newAttr['attribute'] = $attr['attribute']; | |||
| $newAttr['sort'] = $attr['sort']; | |||
| $newAttr['sortLabel'] = $attr['sortLabel']; | |||
| $newAttr['virtualReplica'] = $virtualReplica; | |||
There was a problem hiding this comment.
This should be configurable to allow or disallow customer group pricing sorts as virtual replicas. Otherwise this will balloon the virtual replicas and quickly exhaust the available supply.
| @@ -1555,8 +1556,9 @@ public function handlingReplica($indexName, $storeId, $sortingAttribute = false) | |||
| $sortingIndices = $this->configHelper->getSortingIndices($indexName, $storeId, null, $sortingAttribute); | |||
There was a problem hiding this comment.
We need to rethink how this code is invoked from the configuration backend models which iterates over every store whenever a change is made. Instead the implementation needs to be fully compatible with store scoping and these changes should only impact the affected replicas.
| @@ -1009,7 +1010,14 @@ public function getSortingIndices($originalIndexName, $storeId = null, $currentC | |||
|
|
|||
| $currency = $this->getCurrencyCode($storeId); | |||
| $attributesToAdd = []; | |||
| $defaultVirtualReplicaEnabled = $this->useVirtualReplica($storeId); | |||
There was a problem hiding this comment.
I think the potential for conflicting configurations here will result in confusing/unclear behavior. Since we are now allowing replicas on a per attribute basis - I don't think we actually need this setting.
However, I can see value in having the useVirtualReplica method available in the ConfigHelper object. Instead of having it be a single user configurable yes/no flag it should derive a boolean value by an array operation from the algoliasearch_instant/instant/sorts configuration that searches for any occurrences of a single virtual replica.
| @@ -1555,8 +1556,9 @@ public function handlingReplica($indexName, $storeId, $sortingAttribute = false) | |||
| $sortingIndices = $this->configHelper->getSortingIndices($indexName, $storeId, null, $sortingAttribute); | |||
| if ($this->configHelper->isInstantEnabled($storeId)) { | |||
| $replicas = array_values(array_map(function ($sortingIndex) { | |||
| return $sortingIndex['name']; | |||
| return [$sortingIndex['name'],$sortingIndex['virtualReplica']]; | |||
There was a problem hiding this comment.
While this is a terse way to express the data you are passing, I think it might be more self documenting to use an associative array here - or for that matter just work with the original object without a map operation.
There was a problem hiding this comment.
Another problem is this breaks the array_diff operation later in this function because of the array_merge operation. This can cause an "Array to string conversion" failure when updating replica config. This must be tested across multiple scenarios.
| foreach ($attrs as $key => $attr) { | ||
| if ($virtualReplicaCount < self::MAX_VIRTUAL_REPLICA_COUNT && ($defaultVirtualReplicaEnabled || (isset($attr['virtualReplica']) && $attr['virtualReplica']))){ |
There was a problem hiding this comment.
This boolean logic is almost guaranteed to create confusion based on the current configuration options.
Summary
Result