[
https://issues.apache.org/jira/browse/SOLR-8412?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15064604#comment-15064604
]
Varun Thacker commented on SOLR-8412:
-------------------------------------
Hi Yonik,
{quote}
Reviewing the existing code some, I see this:
- SchemaManager.performOperations() calls doOperations() protected by
schemaUpdateLock
- this performs a list of operations on the latest ManagedIndexSchema object,
which may be created fresh, but will be passed the same schemaUpdateLock
- these operations can call things like addFields()
{quote}
Yes this is what my current understand is as well.
Here is what I additionally gathered. The motivation behind
AddSchemaFieldsUpdateProcessor and SchemaManager are different.
AddSchemaFieldsUpdateProcessor adds one field at a time. So the locking there
is fine.
In SchemaManager we allow bulk operations and want to either perform all the
operations or none.
Now in cloud mode -
SchemaManager#performOperations calls SchemaManager#doOperation which grabs the
latest schema from Zk and performs all the operations on it. Once that it done
it finally tries to save the managed-schema file by calling
ZkController.persistConfigResourceToZooKeeper .
ZkController.persistConfigResourceToZooKeeper will fail if the zkVersion
provided is stale . In that case SchemaManager keeps retrying by fetching the
latest schema from ZK and retrying the entire bulk operation . It does that
till success or timeout .
Thus I feel we don't need any synchronization at all here. Either the current
{[schema.getSchemaUpdateLock()}} or the synchronization on
SchemaManager#performOperations in the new patch.
ZkController.persistConfigResourceToZooKeeper takes care of it.
Now in standalone mode synchronizing on either the current way or the with
making SchemaManager#performOperations synchronized has the same effect?
bq. Bottom line: the synchronization in the current code is complex enough that
I don't know if the proposed simplifications are safe or not. If you could add
some explanation around that, it would be great.
Point taken. On the simplification part here is why I felt the patch simplifies
things - When I first looked at the code I was confused as to how can we take a
lock on a schema object and then fetch the latest schema from ZK and do
operations on that . But later I understood the reasoning . So maybe the patch
simplifies that aspect?
On the correctness part , does my explanation here help? Or you still think
this is not the right way to go? If thats the case I could add comments there
to make the code easier to read and fix the remaining issues I found in another
patch?
> SchemaManager should synchronize on performOperations method
> ------------------------------------------------------------
>
> Key: SOLR-8412
> URL: https://issues.apache.org/jira/browse/SOLR-8412
> Project: Solr
> Issue Type: Bug
> Reporter: Varun Thacker
> Priority: Minor
> Attachments: SOLR-8412.patch, SOLR-8412.patch, SOLR-8412.patch
>
>
> Currently SchemaManager synchronizes on {{schema.getSchemaUpdateLock()}} . We
> should synzhronize on {{performOperations}} instead.
> The net affect will be the same but the code will be more clear.
> {{schema.getSchemaUpdateLock()}} is used when you want to edit a schema and
> add one field at a time. But the way SchemaManager works is that it does bulk
> operations i.e performs all operations and then persists the final schema .
> If there were two concurrent operations that took place, the later operation
> will retry by fetching the latest schema .
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
