You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "Bilal Waheed (Jira)" <ji...@apache.org> on 2019/11/26 00:26:00 UTC
[jira] [Comment Edited] (SOLR-13932) Review directory locking and
Blob interactions
[ https://issues.apache.org/jira/browse/SOLR-13932?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16982001#comment-16982001 ]
Bilal Waheed edited comment on SOLR-13932 at 11/26/19 12:25 AM:
----------------------------------------------------------------
_>>Pushes should instead be done from the existing index directory without making copies..._
Agreed, with commit point reservation logic, no need to take snapshot.
We can get rid of storing a generation number but we cannot get rid of reasoning about it.
Pull needs it to be functionally correct. Push needs it for an optimization.
[https://github.com/apache/lucene-solr/blob/9201c586ba05537a049e6406070361cdda561a4c/solr/core/src/java/org/apache/solr/store/blob/client/BlobCoreMetadata.java#L29] talks about how to get rid of storing generation number and what are the alternates.
_>>Decision to pull from Blob should only be based on differences between local commit point and Blob set of files..._
This needs to take care of a scenario where local generation number is higher than blob's. Not storing generation number is ok but it needs to be inferred and involved in the comparison decision(mere presence of higher generation number locally is a conflict, higher is considered active/source of truth [https://lucene.apache.org/core/3_0_3/fileformats.html#Segments%20File]). Without reasoning about higher generation number, those higher generation files individually would not conflict with any other file and we will not know that we need to pull into a new index directory.
_>>CorePusher.pushCoreToBlob() that compares the generation of local commit point to the Blob generation should go away..._
This optimization is saving us from unnecessarily creating ServerSideMetadata(reserve commit point, comparison etc.) when we know that there would be nothing to push since some other index batch already pushed on our behalf. We are piggybacking on cached BlobCoreMetadata's generation number here. A more accurate representation of this optimization would be to add a "lastGenerationPushed" property to per core cache(SharedCoreConcurrencyController#coresVersionMetadata) and update it to whatever generation gets pushed successfully and to -1(unlikely value) on each successful pull. [https://github.com/apache/lucene-solr/blob/9201c586ba05537a049e6406070361cdda561a4c/solr/core/src/java/org/apache/solr/store/blob/process/CorePusher.java#L104] has more notes.
Four days ago, I added bunch of notes in [https://github.com/apache/lucene-solr/pull/1028]. I am not sure if you are sync with that. The notes I referred above are mainly from that PR.
Edit: fixed hyperlink
was (Author: mbwaheed):
_>>Pushes should instead be done from the existing index directory without making copies..._
Agreed, with commit point reservation logic, no need to take snapshot.
We can get rid of storing a generation number but we cannot get rid of reasoning about it.
Pull needs it to be functionally correct. Push needs it for an optimization.
https://github.com/apache/lucene-solr/blob/9201c586ba05537a049e6406070361cdda561a4c/solr/core/src/java/org/apache/solr/store/blob/client/BlobCoreMetadata.java#L29 talks about how to get rid of storing generation number and what are the alternates.
_>>Decision to pull from Blob should only be based on differences between local commit point and Blob set of files..._
This needs to take care of a scenario where local generation number is higher than blob's. Not storing generation number is ok but it needs to be inferred and involved in the comparison decision(mere presence of higher generation number locally is a conflict, [higher is considered active/source of truth|[https://lucene.apache.org/core/3_0_3/fileformats.html#Segments%20File]]). Without reasoning about higher generation number, those higher generation files individually would not conflict with any other file and we will not know that we need to pull into a new index directory.
_>>CorePusher.pushCoreToBlob() that compares the generation of local commit point to the Blob generation should go away..._
This optimization is saving us from unnecessarily creating ServerSideMetadata(reserve commit point, comparison etc.) when we know that there would be nothing to push since some other index batch already pushed on our behalf. We are piggybacking on cached BlobCoreMetadata's generation number here. A more accurate representation of this optimization would be to add a "lastGenerationPushed" property to per core cache(SharedCoreConcurrencyController#coresVersionMetadata) and update it to whatever generation gets pushed successfully and to -1(unlikely value) on each successful pull. https://github.com/apache/lucene-solr/blob/9201c586ba05537a049e6406070361cdda561a4c/solr/core/src/java/org/apache/solr/store/blob/process/CorePusher.java#L104 has more notes.
Four days ago, I added bunch of notes in https://github.com/apache/lucene-solr/pull/1028. I am not sure if you are sync with that. The notes I referred above are mainly from that PR.
> Review directory locking and Blob interactions
> ----------------------------------------------
>
> Key: SOLR-13932
> URL: https://issues.apache.org/jira/browse/SOLR-13932
> Project: Solr
> Issue Type: Sub-task
> Reporter: Ilan Ginzburg
> Priority: Major
>
> Review resolution of local index directory content vs Blob copy.
> There has been wrong understanding of following line acquiring a lock on index directory.
> {{solrCore.getDirectoryFactory().get(indexDirPath, DirectoryFactory.DirContext.DEFAULT, solrCore.getSolrConfig().indexConfig.lockType);}}
> From Yonik:
> _A couple things about Directory locking.... the locks were only ever to prevent more than one IndexWriter from trying to modify the same index. The IndexWriter grabs a write lock once when it is created and does not release it until it is closed._
> _Directories are not locked on acquisition of the Directory from the DirectoryFactory. See the IndexWriter constructor, where the lock is explicitly grabbed._
> Review CorePushPull#pullUpdateFromBlob, ServerSideMetadata and other classes as relevant.
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org