You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2019/12/03 16:51:02 UTC
[GitHub] [lucene-solr] yonik commented on a change in pull request #1055:
SOLR-13932 Review directory locking and Blob interactions
yonik commented on a change in pull request #1055: SOLR-13932 Review directory locking and Blob interactions
URL: https://github.com/apache/lucene-solr/pull/1055#discussion_r353298728
##########
File path: solr/core/src/java/org/apache/solr/store/blob/metadata/CorePushPull.java
##########
@@ -96,81 +96,77 @@ protected CoreContainer getContainerFromServerMetadata(ServerSideMetadata solrSe
*/
public BlobCoreMetadata pushToBlobStore(String currentMetadataSuffix, String newMetadataSuffix) throws Exception {
long startTimeMs = System.currentTimeMillis();
+ SolrCore solrCore = container.getCore(pushPullData.getCoreName());
+ if (solrCore == null) {
+ throw new Exception("Can't find core " + pushPullData.getCoreName());
+ }
+
try {
- SolrCore solrCore = container.getCore(pushPullData.getCoreName());
- if (solrCore == null) {
- throw new Exception("Can't find core " + pushPullData.getCoreName());
+ // Creating the new BlobCoreMetadata as a modified clone of the existing one
+ BlobCoreMetadataBuilder bcmBuilder = new BlobCoreMetadataBuilder(blobMetadata, solrServerMetadata.getGeneration());
+
+ /*
+ * Removing from the core metadata the files that are stored on the blob store but no longer needed.
+ *
+ * When this method is executed, the content of the index on Blob is to be replaced with the local content.
+ * The assumption (or normal flow) is for local to refresh from Blob, update locally then push the
+ * changes to Blob.
+ * When merges happen locally the update has to mark old segment files for delete (and for example the
+ * previous "segments_N" is to be deleted as a new higher generation segment has been created).
+ */
+ for (BlobCoreMetadata.BlobFile d : resolvedMetadataResult.getFilesToDelete()) {
+ bcmBuilder.removeFile(d);
+ BlobCoreMetadata.BlobFileToDelete bftd = new BlobCoreMetadata.BlobFileToDelete(d, System.currentTimeMillis());
+ bcmBuilder.addFileToDelete(bftd);
}
- try {
- // Creating the new BlobCoreMetadata as a modified clone of the existing one
- BlobCoreMetadataBuilder bcmBuilder = new BlobCoreMetadataBuilder(blobMetadata, solrServerMetadata.getGeneration());
-
- Directory snapshotIndexDir = solrCore.getDirectoryFactory().get(solrServerMetadata.getSnapshotDirPath(), DirectoryFactory.DirContext.DEFAULT, solrCore.getSolrConfig().indexConfig.lockType);
- try {
+ // add the old core.metadata file to delete
+ if (!currentMetadataSuffix.equals(SharedShardMetadataController.METADATA_NODE_DEFAULT_VALUE)) {
+ // TODO This may be inefficient but we'll likely remove this when CorePushPull is refactored to have deletion elsewhere
+ // could be added to resolvedMetadataResult#getFilesToDelete()
+ ToFromJson<BlobCoreMetadata> converter = new ToFromJson<>();
+ String json = converter.toJson(blobMetadata);
+ int bcmSize = json.getBytes().length;
+
+ String blobCoreMetadataName = BlobStoreUtils.buildBlobStoreMetadataName(currentMetadataSuffix);
+ String coreMetadataPath = blobMetadata.getSharedBlobName() + "/" + blobCoreMetadataName;
+ // so far checksum is not used for metadata file
+ BlobCoreMetadata.BlobFileToDelete bftd = new BlobCoreMetadata.BlobFileToDelete("", coreMetadataPath, bcmSize, BlobCoreMetadataBuilder.UNDEFINED_VALUE, System.currentTimeMillis());
+ bcmBuilder.addFileToDelete(bftd);
+ }
- /*
- * Removing from the core metadata the files that should no longer be there.
- *
- * TODO
- * This is a little confusing: This is equivalent to what we were doing in first-party
- * where the files to delete for a push were just the files that we determined were
- * missing locally but on blob (filesToPull) for a push. This operates on the assumption
- * that the core locally was refreshed with what was in blob before this update (both in
- * first party and Solr Cloud).
- *
- * SharedMetadataResolutionResult makes no distinction between what action is being taken
- * (push or pull) hence the confusing method naming but leaving this for now while we reach
- * blob feature parity.
- *
- * The deletion logic will move out of this class in the future and make this less confusing.
- */
- for (BlobCoreMetadata.BlobFile d : resolvedMetadataResult.getFilesToDelete()) {
- bcmBuilder.removeFile(d);
- BlobCoreMetadata.BlobFileToDelete bftd = new BlobCoreMetadata.BlobFileToDelete(d, System.currentTimeMillis());
- bcmBuilder.addFileToDelete(bftd);
- }
-
- // add the old core.metadata file to delete
- if (!currentMetadataSuffix.equals(SharedShardMetadataController.METADATA_NODE_DEFAULT_VALUE)) {
- // TODO This may be inefficient but we'll likely remove this when CorePushPull is refactored to have deletion elsewhere
- // could be added to resolvedMetadataResult#getFilesToDelete()
- ToFromJson<BlobCoreMetadata> converter = new ToFromJson<>();
- String json = converter.toJson(blobMetadata);
- int bcmSize = json.getBytes().length;
-
- String blobCoreMetadataName = BlobStoreUtils.buildBlobStoreMetadataName(currentMetadataSuffix);
- String coreMetadataPath = blobMetadata.getSharedBlobName() + "/" + blobCoreMetadataName;
- // so far checksum is not used for metadata file
- BlobCoreMetadata.BlobFileToDelete bftd = new BlobCoreMetadata.BlobFileToDelete("", coreMetadataPath, bcmSize, BlobCoreMetadataBuilder.UNDEFINED_VALUE, System.currentTimeMillis());
- bcmBuilder.addFileToDelete(bftd);
- }
-
- // Directory's javadoc says: "Java's i/o APIs not used directly, but rather all i/o is through this API"
- // But this is untrue/totally false/misleading. SnapPuller has File all over.
- for (CoreFileData cfd : resolvedMetadataResult.getFilesToPush()) {
- // Sanity check that we're talking about the same file (just sanity, Solr doesn't update files so should never be different)
- assert cfd.getFileSize() == snapshotIndexDir.fileLength(cfd.getFileName());
-
- String blobPath = pushFileToBlobStore(coreStorageClient, snapshotIndexDir, cfd.getFileName(), cfd.getFileSize());
- bcmBuilder.addFile(new BlobCoreMetadata.BlobFile(cfd.getFileName(), blobPath, cfd.getFileSize(), cfd.getChecksum()));
- }
- } finally {
- solrCore.getDirectoryFactory().release(snapshotIndexDir);
+ // When we build solrServerMetadata we requested to reserve the commit point for some short duration. Assumption is
+ // it took less than this duration to get here (didn't do anything blocking) and now we actually save the commit
+ // point for the (pontentially long) time it takes to push all files to the Blob store.
Review comment:
"pontentially" spelling bug
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org