You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/02/04 13:11:59 UTC
[GitHub] [solr] ijioio opened a new pull request #595: Add end time value to backup response
ijioio opened a new pull request #595:
URL: https://github.com/apache/solr/pull/595
https://issues.apache.org/jira/browse/SOLR-15982
# Description
Adding new field `endTime` to collection backup response (response is actually an aggregated result of responses from multiple nodes).
# Solution
After backup finished (regardless is it sync or async) it will write `backup.properties` file to repository. Actually it writes the content of the `org.apache.solr.core.backup.BackupProperties`. Before writing backup properties data the value `endTime` is filled at `org.apache.solr.core.backup.BackupProperties.store(Writer)`:
```java
public void store(Writer propsWriter) throws IOException {
properties.put("indexSizeMB", String.valueOf(indexSizeMB));
properties.put("indexFileCount", String.valueOf(indexFileCount));
properties.put(BackupManager.END_TIME_PROP, Instant.now().toString());
properties.store(propsWriter, "Backup properties file");
}
```
Exactly after writing backup properties file an additional field `endTime` will be appended to the response within `org.apache.solr.cloud.api.collections.BackupCmd.call(ClusterState, ZkNodeProps, NamedList<Object>)` call:
```java
public void call(ClusterState state, ZkNodeProps message, NamedList<Object> results) throws Exception {
...
try (BackupRepository repository = cc.newBackupRepository(repo)) {
...
backupMgr.writeBackupProperties(backupProperties);
if(backupProperties != null) {
NamedList<Object> response = (NamedList<Object>) results.get("response");
response.add("endTime", backupProperties.getEndTime());
}
...
}
}
```
# Tests
None
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r802960236
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java
##########
@@ -138,6 +138,15 @@ public void call(ClusterState state, ZkNodeProps message, NamedList<Object> resu
backupMgr.writeBackupProperties(backupProperties);
+ // It can't be done within aggregateResults call
+ // since endTime is filled later
+ if(backupProperties != null) {
Review comment:
Ah, sorry, I misunderstood you. Yeah, sure, at that point it is always non null. Will remove the check...
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r809434230
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java
##########
@@ -116,6 +116,10 @@ public void call(ClusterState state, ZkNodeProps message, NamedList<Object> resu
break;
}
case CollectionAdminParams.NO_INDEX_BACKUP_STRATEGY: {
+ NamedList<Object> response = new SimpleOrderedMap<>();
+ response.add("collection", collectionName);
+ response.add("startTime", backupProperties.getStartTime());
+ results.add("response", response);
Review comment:
Good catch! I have totally forgot about this strategy %(
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1034132999
Just in case, examples of full responses of snapshot backup (before and after changes applied):
Before:
```json
{
"responseHeader": {
"status": 0,
"QTime": 0
},
"success": {
"localhost:8983_solr": {
"responseHeader": {
"status": 0,
"QTime": 1
}
},
"localhost:8983_solr": {
"responseHeader": {
"status": 0,
"QTime": 0
}
},
"localhost:8983_solr": {
"responseHeader": {
"status": 0,
"QTime": 0
},
"STATUS": "completed",
"msg": "TaskId: 1004319510579040300 webapp=null path=/admin/cores params={core=techproducts_shard2_replica_n1&async=1004319510579040300&qt=/admin/cores&name=shard2&action=BACKUPCORE&location=file:///path/to/my/shared/drive/mybackup/techproducts&incremental=false&wt=javabin&version=2} status=0 QTime=1",
"response": {
"startTime": "Wed Feb 09 07:25:51 UTC 2022",
"fileCount": 1,
"status": "success",
"snapshotCompletedAt": "Wed Feb 09 07:25:51 UTC 2022",
"snapshotName": "shard2",
"directoryName": "snapshot.shard2"
}
},
"localhost:8983_solr": {
"responseHeader": {
"status": 0,
"QTime": 0
},
"STATUS": "completed",
"msg": "TaskId: 1004319510579566200 webapp=null path=/admin/cores params={core=techproducts_shard1_replica_n6&async=1004319510579566200&qt=/admin/cores&name=shard1&action=BACKUPCORE&location=file:///path/to/my/shared/drive/mybackup/techproducts&incremental=false&wt=javabin&version=2} status=0 QTime=0",
"response": {
"startTime": "Wed Feb 09 07:25:51 UTC 2022",
"fileCount": 21,
"status": "success",
"snapshotCompletedAt": "Wed Feb 09 07:25:51 UTC 2022",
"snapshotName": "shard1",
"directoryName": "snapshot.shard1"
}
}
},
"1004319510579040300": {
"responseHeader": {
"status": 0,
"QTime": 0
},
"STATUS": "completed",
"msg": "TaskId: 1004319510579040300 webapp=null path=/admin/cores params={core=techproducts_shard2_replica_n1&async=1004319510579040300&qt=/admin/cores&name=shard2&action=BACKUPCORE&location=file:///path/to/my/shared/drive/mybackup/techproducts&incremental=false&wt=javabin&version=2} status=0 QTime=1",
"response": {
"startTime": "Wed Feb 09 07:25:51 UTC 2022",
"fileCount": 1,
"status": "success",
"snapshotCompletedAt": "Wed Feb 09 07:25:51 UTC 2022",
"snapshotName": "shard2",
"directoryName": "snapshot.shard2"
}
},
"1004319510579566200": {
"responseHeader": {
"status": 0,
"QTime": 0
},
"STATUS": "completed",
"msg": "TaskId: 1004319510579566200 webapp=null path=/admin/cores params={core=techproducts_shard1_replica_n6&async=1004319510579566200&qt=/admin/cores&name=shard1&action=BACKUPCORE&location=file:///path/to/my/shared/drive/mybackup/techproducts&incremental=false&wt=javabin&version=2} status=0 QTime=0",
"response": {
"startTime": "Wed Feb 09 07:25:51 UTC 2022",
"fileCount": 21,
"status": "success",
"snapshotCompletedAt": "Wed Feb 09 07:25:51 UTC 2022",
"snapshotName": "shard1",
"directoryName": "snapshot.shard1"
}
},
"response": {
"collection": "techproducts",
"numShards": 2,
"endTime": "2022-02-09T07:25:52.879269500Z"
},
"status": {
"state": "completed",
"msg": "found [1004] in completed tasks"
}
}
```
After:
```json
{
"responseHeader": {
"status": 0,
"QTime": 2
},
"success": {
"localhost:8983_solr": {
"responseHeader": {
"status": 0,
"QTime": 1
}
},
"localhost:8983_solr": {
"responseHeader": {
"status": 0,
"QTime": 1
}
},
"localhost:8983_solr": {
"responseHeader": {
"status": 0,
"QTime": 0
},
"STATUS": "completed",
"msg": "TaskId: 1000362128923089600 webapp=null path=/admin/cores params={core=techproducts_shard2_replica_n6&async=1000362128923089600&qt=/admin/cores&name=shard2&action=BACKUPCORE&location=file:///path/to/my/shared/drive/mybackup/techproducts&incremental=false&wt=javabin&version=2} status=0 QTime=1",
"response": {
"startTime": "Wed Feb 09 19:16:10 UTC 2022",
"fileCount": 1,
"status": "success",
"snapshotCompletedAt": "Wed Feb 09 19:16:10 UTC 2022",
"snapshotName": "shard2",
"directoryName": "snapshot.shard2"
}
},
"localhost:8983_solr": {
"responseHeader": {
"status": 0,
"QTime": 0
},
"STATUS": "completed",
"msg": "TaskId: 1000362128925570500 webapp=null path=/admin/cores params={core=techproducts_shard1_replica_n4&async=1000362128925570500&qt=/admin/cores&name=shard1&action=BACKUPCORE&location=file:///path/to/my/shared/drive/mybackup/techproducts&incremental=false&wt=javabin&version=2} status=0 QTime=1",
"response": {
"startTime": "Wed Feb 09 19:16:10 UTC 2022",
"fileCount": 21,
"status": "success",
"snapshotCompletedAt": "Wed Feb 09 19:16:10 UTC 2022",
"snapshotName": "shard1",
"directoryName": "snapshot.shard1"
}
}
},
"1000362128923089600": {
"responseHeader": {
"status": 0,
"QTime": 0
},
"STATUS": "completed",
"msg": "TaskId: 1000362128923089600 webapp=null path=/admin/cores params={core=techproducts_shard2_replica_n6&async=1000362128923089600&qt=/admin/cores&name=shard2&action=BACKUPCORE&location=file:///path/to/my/shared/drive/mybackup/techproducts&incremental=false&wt=javabin&version=2} status=0 QTime=1",
"response": {
"startTime": "Wed Feb 09 19:16:10 UTC 2022",
"fileCount": 1,
"status": "success",
"snapshotCompletedAt": "Wed Feb 09 19:16:10 UTC 2022",
"snapshotName": "shard2",
"directoryName": "snapshot.shard2"
}
},
"1000362128925570500": {
"responseHeader": {
"status": 0,
"QTime": 0
},
"STATUS": "completed",
"msg": "TaskId: 1000362128925570500 webapp=null path=/admin/cores params={core=techproducts_shard1_replica_n4&async=1000362128925570500&qt=/admin/cores&name=shard1&action=BACKUPCORE&location=file:///path/to/my/shared/drive/mybackup/techproducts&incremental=false&wt=javabin&version=2} status=0 QTime=1",
"response": {
"startTime": "Wed Feb 09 19:16:10 UTC 2022",
"fileCount": 21,
"status": "success",
"snapshotCompletedAt": "Wed Feb 09 19:16:10 UTC 2022",
"snapshotName": "shard1",
"directoryName": "snapshot.shard1"
}
},
"response": {
"collection": "techproducts",
"numShards": 2,
"backupId": -1,
"indexVersion": "9.0.0",
"startTime": "2022-02-09T19:16:10.082015800Z",
"indexFileCount": 22,
"uploadedIndexFileCount": 22,
"endTime": "2022-02-09T19:16:11.191537400Z"
},
"status": {
"state": "completed",
"msg": "found [1000] in completed tasks"
}
}
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1036578775
> How you think, maybe we should do the same trick for delete snapshot also? I mean adding field startTime (it is not added for some reason) and adding field endTime to duplicate snapshotDeletedAt?
Done
> This looks good to me. Maybe instead of TODO: put DEPRECATED:, gives a better indication of future action.
Sure! Updated...
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on pull request #595: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1029976420
An alternative is to initialize and fill `endTime` value within the `aggregateResults` call, but in that case time spend for downloading/copying config will be not taken into account as it is happens **_after_** backup of index files and `aggregateResults` call.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r805908626
##########
File path: solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-9.adoc
##########
@@ -104,6 +104,44 @@ All the usages are replaced by BaseHttpSolrClient.RemoteSolrException and BaseHt
In previous versions there was a field returned in async backup status responses, `Response`. This has now been renamed to `msg`, to better fit other collections API responses.
The `response` field is now a map, containing information about the backup (`startTime`, `indexSizeMB`, `indexFileCount`, etc.).
+* SOLR-15982: For collection's snapshot backup request responses additional fields `indexVersion`, `indexFileCount`, etc. were added similar to incremental backup request responses. Also, both snapshot and incremental backup request responses will now contain `starTime` and `endTime`. Here is an example of how collection's snapshot backup request responses will looks like:
+
+Incremental:
+
+```json
+"response": {
+ "collection": "techproducts",
+ "numShards": 2,
+ "backupId": 0,
+ "indexVersion": "9.0.0",
+ "startTime": "2022-02-11T17:20:44.157305500Z",
+ "indexFileCount": 22,
+ "uploadedIndexFileCount": 22,
+ "indexSizeMB": 0.007,
+ "uploadedIndexFileMB": 0.007,
+ "shardBackupIds": [
+ "md_shard2_0",
+ "md_shard1_0"
+ ],
+ "endTime": "2022-02-11T17:20:45.245534400Z"
+}
+```
+
+Snapshot:
+
+```json
+"response": {
+ "collection": "techproducts",
+ "numShards": 2,
+ "indexVersion": "9.0.0",
+ "startTime": "2022-02-11T17:19:33.271461700Z",
+ "indexFileCount": 22,
+ "endTime": "2022-02-11T17:19:34.363859100Z"
+}
+```
Review comment:
> Suggest to omit the example here and to update and/or extend the "Output Snippet" in https://github.com/apache/solr/blob/main/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc#backup-status instead.
I'm glad to, but it seems this is a page about Core API operations, not Collection API operations. I've checked the page for Collection API, but they doesn't have any examples of responses, so nothing to update there. If you know other places in documentation please let me know.
Concerning backup-status document, I will check the shard status response, if it has changed, I will update the document...
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r804884691
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java
##########
@@ -298,7 +306,7 @@ private void incrementalCopyIndexFiles(URI backupUri, String collectionName, ZkN
uploadedIndexFileCount = Optional.of(uploadedIndexFileCount.orElse(0) + shardUploadedIndexFileCount);
}
Double shardIndexSizeMB = (Double) shardResp.get("indexSizeMB");
- if (shardUploadedIndexFileCount != null) {
+ if (shardIndexSizeMB != null) {
Review comment:
good catch!
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java
##########
@@ -289,7 +294,10 @@ private void incrementalCopyIndexFiles(URI backupUri, String collectionName, ZkN
NamedList<?> shardResp = (NamedList<?>)((NamedList<?>)shards.getVal(i)).get("response");
if (shardResp == null)
continue;
- Integer shardIndexFileCount = (Integer) shardResp.get("indexFileCount");
+ // indexFileCount is expected to be found in case of incremental backups
+ // fileCount is expected to be found in case of snapshot backups
+ Integer shardIndexFileCount = (Integer) Optional.<Object> ofNullable(shardResp.get("indexFileCount"))
+ .orElse(shardResp.get("fileCount"));
Review comment:
```suggestion
Integer shardIndexFileCount = (Integer) shardResp.get(incremental ? "indexFileCount" : "fileCount");
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r806052548
##########
File path: solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-9.adoc
##########
@@ -104,6 +104,44 @@ All the usages are replaced by BaseHttpSolrClient.RemoteSolrException and BaseHt
In previous versions there was a field returned in async backup status responses, `Response`. This has now been renamed to `msg`, to better fit other collections API responses.
The `response` field is now a map, containing information about the backup (`startTime`, `indexSizeMB`, `indexFileCount`, etc.).
+* SOLR-15982: For collection's snapshot backup request responses additional fields `indexVersion`, `indexFileCount`, etc. were added similar to incremental backup request responses. Also, both snapshot and incremental backup request responses will now contain `starTime` and `endTime`. Here is an example of how collection's snapshot backup request responses will looks like:
+
+Incremental:
+
+```json
+"response": {
+ "collection": "techproducts",
+ "numShards": 2,
+ "backupId": 0,
+ "indexVersion": "9.0.0",
+ "startTime": "2022-02-11T17:20:44.157305500Z",
+ "indexFileCount": 22,
+ "uploadedIndexFileCount": 22,
+ "indexSizeMB": 0.007,
+ "uploadedIndexFileMB": 0.007,
+ "shardBackupIds": [
+ "md_shard2_0",
+ "md_shard1_0"
+ ],
+ "endTime": "2022-02-11T17:20:45.245534400Z"
+}
+```
+
+Snapshot:
+
+```json
+"response": {
+ "collection": "techproducts",
+ "numShards": 2,
+ "indexVersion": "9.0.0",
+ "startTime": "2022-02-11T17:19:33.271461700Z",
+ "indexFileCount": 22,
+ "endTime": "2022-02-11T17:19:34.363859100Z"
+}
+```
Review comment:
I agree that this is too much for the major changes page. I think you can add it to the collections api entry if you want, and style it like this: https://solr.apache.org/guide/8_11/collections-api.html#examples-of-async-requests, but it's not a requirement for this PR.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r800892888
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java
##########
@@ -138,6 +138,15 @@ public void call(ClusterState state, ZkNodeProps message, NamedList<Object> resu
backupMgr.writeBackupProperties(backupProperties);
+ // It can't be done within aggregateResults call
+ // since endTime is filled later
+ if(backupProperties != null) {
+ // It is safe to extract results here
+ @SuppressWarnings("unchecked")
+ NamedList<Object> response = (NamedList<Object>) results.get("response");
+ response.add("endTime", backupProperties.getEndTime());
Review comment:
`backupProperties.getEndTime()` would return `null` if it hadn't been filled in later for some reason; that could be null-checked but that would add complexity and so this is good; yet more complicated would be for `backupMgr.writeBackupProperties` to somehow return properties which it added or updated but that would be overcomplex and so this here is good.
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java
##########
@@ -138,6 +138,15 @@ public void call(ClusterState state, ZkNodeProps message, NamedList<Object> resu
backupMgr.writeBackupProperties(backupProperties);
+ // It can't be done within aggregateResults call
+ // since endTime is filled later
+ if(backupProperties != null) {
Review comment:
Within `aggregateResults` the backup properties are nullable but here it (currently) seems always present so perhaps the null-check here could be omitted?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1036501713
This looks good to me. Maybe instead of `TODO:` put `DEPRECATED:`, gives a better indication of future action.
Otherwise I think this is probably good to go once the docs are updated, there is a changelog entry for 9.0, and some upgrade notes in `major-changes-in-solr-9.adoc`, though that can be brief.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1036382541
Beyond what Christine has said, I have a few questions:
- I believe `snapshotCompletedAt` is a new field, correct? If so why not name it `endTime` to match the incremental backup response?
- I notice that there is a lot of mixing of date formats. Since this is a major version, can we go ahead and start using the ISO date format for everything? I think I noticed this was documented somewhere when I was working on the previous ticket... Ok looking at it, there are comments a few places saying that it should be changed from `new Date().toString()` to `Instant.now().toString()`. That should do the trick, if we make sure to use that throughout the backup code.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on pull request #595: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1029979243
This is an example of response containing `endTime` field:
```json
{
"responseHeader": {
"status": 0,
"QTime": 1
},
"success": {
"localhost:8983_solr": {
"responseHeader": {
"status": 0,
"QTime": 1
}
},
"localhost:8983_solr": {
"responseHeader": {
"status": 0,
"QTime": 1
}
},
"localhost:8983_solr": {
"responseHeader": {
"status": 0,
"QTime": 0
},
"STATUS": "completed",
"msg": "TaskId: 1001701080135333600 webapp=null path=/admin/cores params={core=techproducts_shard2_replica_n1&async=1001701080135333600&qt=/admin/cores&name=shard2&shardBackupId=md_shard2_0&action=BACKUPCORE&location=file:///path/to/my/shared/drive/mybackup/techproducts&incremental=true&wt=javabin&version=2} status=0 QTime=1",
"response": {
"startTime": "2022-02-04T12:35:52.304582700Z",
"indexFileCount": 1,
"uploadedIndexFileCount": 1,
"indexSizeMB": 0.001,
"uploadedIndexFileMB": 0.001,
"shard": "shard2",
"endTime": "2022-02-04T12:35:52.317049900Z",
"shardBackupId": "md_shard2_0"
}
},
"localhost:8983_solr": {
"responseHeader": {
"status": 0,
"QTime": 0
},
"STATUS": "completed",
"msg": "TaskId: 1001701080136733900 webapp=null path=/admin/cores params={core=techproducts_shard1_replica_n6&async=1001701080136733900&qt=/admin/cores&name=shard1&shardBackupId=md_shard1_0&action=BACKUPCORE&location=file:///path/to/my/shared/drive/mybackup/techproducts&incremental=true&wt=javabin&version=2} status=0 QTime=1",
"response": {
"startTime": "2022-02-04T12:35:52.304582700Z",
"indexFileCount": 21,
"uploadedIndexFileCount": 21,
"indexSizeMB": 0.006,
"uploadedIndexFileMB": 0.006,
"shard": "shard1",
"endTime": "2022-02-04T12:35:52.348963900Z",
"shardBackupId": "md_shard1_0"
}
}
},
"1001701080135333600": {
"responseHeader": {
"status": 0,
"QTime": 0
},
"STATUS": "completed",
"msg": "TaskId: 1001701080135333600 webapp=null path=/admin/cores params={core=techproducts_shard2_replica_n1&async=1001701080135333600&qt=/admin/cores&name=shard2&shardBackupId=md_shard2_0&action=BACKUPCORE&location=file:///path/to/my/shared/drive/mybackup/techproducts&incremental=true&wt=javabin&version=2} status=0 QTime=1",
"response": {
"startTime": "2022-02-04T12:35:52.304582700Z",
"indexFileCount": 1,
"uploadedIndexFileCount": 1,
"indexSizeMB": 0.001,
"uploadedIndexFileMB": 0.001,
"shard": "shard2",
"endTime": "2022-02-04T12:35:52.317049900Z",
"shardBackupId": "md_shard2_0"
}
},
"1001701080136733900": {
"responseHeader": {
"status": 0,
"QTime": 0
},
"STATUS": "completed",
"msg": "TaskId: 1001701080136733900 webapp=null path=/admin/cores params={core=techproducts_shard1_replica_n6&async=1001701080136733900&qt=/admin/cores&name=shard1&shardBackupId=md_shard1_0&action=BACKUPCORE&location=file:///path/to/my/shared/drive/mybackup/techproducts&incremental=true&wt=javabin&version=2} status=0 QTime=1",
"response": {
"startTime": "2022-02-04T12:35:52.304582700Z",
"indexFileCount": 21,
"uploadedIndexFileCount": 21,
"indexSizeMB": 0.006,
"uploadedIndexFileMB": 0.006,
"shard": "shard1",
"endTime": "2022-02-04T12:35:52.348963900Z",
"shardBackupId": "md_shard1_0"
}
},
"response": {
"collection": "techproducts",
"numShards": 2,
"backupId": 0,
"indexVersion": "9.0.0",
"startTime": "2022-02-04T12:35:52.282616800Z",
"indexFileCount": 22,
"uploadedIndexFileCount": 22,
"indexSizeMB": 0.007,
"uploadedIndexFileMB": 0.007,
"shardBackupIds": [
"md_shard2_0",
"md_shard1_0"
],
"endTime": "2022-02-04T12:35:53.400626Z"
},
"status": {
"state": "completed",
"msg": "found [1001] in completed tasks"
}
}
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r801624967
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java
##########
@@ -138,6 +138,15 @@ public void call(ClusterState state, ZkNodeProps message, NamedList<Object> resu
backupMgr.writeBackupProperties(backupProperties);
+ // It can't be done within aggregateResults call
+ // since endTime is filled later
+ if(backupProperties != null) {
Review comment:
Interesting. But even with the existing `copyIndexFiles` logic, could we not simplify here:
```suggestion
// It can't be done within aggregateResults call
// since endTime is filled later
{
```
because earlier at line 89 we always have
```
BackupProperties backupProperties = BackupProperties.create(backupName, collectionName,
extCollectionName, configName);
```
i.e. `backupProperties` never is null?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r801550139
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java
##########
@@ -138,6 +138,15 @@ public void call(ClusterState state, ZkNodeProps message, NamedList<Object> resu
backupMgr.writeBackupProperties(backupProperties);
+ // It can't be done within aggregateResults call
+ // since endTime is filled later
+ if(backupProperties != null) {
Review comment:
Hi @cpoerschke Thanks for the note! It can be `null`, in case of non incremental call it will be redirected to `copyIndexFiles(URI, String, ZkNodeProps, NamedList<Object>)`:
```java
public void call(ClusterState state, ZkNodeProps message, NamedList<Object> results) throws Exception {
...
try (BackupRepository repository = cc.newBackupRepository(repo)) {
...
if (incremental) {
incrementalCopyIndexFiles(backupUri, collectionName, message, results, backupProperties, backupMgr);
} else {
copyIndexFiles(backupUri, collectionName, message, results);
}
...
}
...
}
```
and within `copyIndexFiles` it will call `aggregateResults` without providing `backupManager` and `backupProps`:
```java
private void copyIndexFiles(URI backupPath, String collectionName, ZkNodeProps request, NamedList<Object> results) throws Exception {
...
NamedList<Object> aggRsp = aggregateResults(results, collectionName, slices, null, null);
...
}
```
Maybe we should investigate this is it safe to provide `copyIndexFiles` with `backupManager` and `backupProps`?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1042023456
@HoustonPutman Oh, I was going to cleanup/update docs today but realized you have already done it,. Thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1041914290
Ok I think this is good to go. Will merge and backport later today if you don't have any other notes @cpoerschke
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1036531144
> OMG, it seems I messed up with conflict merge in Eclipse...
It looks like the merge commit swallowed up all the other commits on main...
Try resetting your head to before the merge commit (the last commit on this branch), then force pushing. Then just do the merge via the Github UI, since it'll be a small conflict.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1034127420
@cpoerschke @HoustonPutman
> Maybe we should investigate this is it safe to provide copyIndexFiles with backupManager and backupProps?
I've updated signature of `copyIndexFiles` method for non incremental (snapshot) backup to accept `backupManager` and `backupProperties`. In turn, it will pass them both to `aggregateResults`. This way `aggregateResults` will always get non null `backupManager` and `backupProperties` values, so it will be safe to extract necessary data from it. First of all we will get `startTime` (before it was `null` for snapshot backups).
Second, let's compare shard responses of incremental and snapshot backups:
Incremental:
```json
"response": {
"startTime": "2022-02-04T12:35:52.304582700Z",
"indexFileCount": 21,
"uploadedIndexFileCount": 21,
"indexSizeMB": 0.006,
"uploadedIndexFileMB": 0.006,
"shard": "shard1",
"endTime": "2022-02-04T12:35:52.348963900Z",
"shardBackupId": "md_shard1_0"
}
```
Snapshot:
```json
"response": {
"startTime": "Wed Feb 09 07:25:51 UTC 2022",
"fileCount": 21,
"status": "success",
"snapshotCompletedAt": "Wed Feb 09 07:25:51 UTC 2022",
"snapshotName": "shard1",
"directoryName": "snapshot.shard1"
}
```
They are not equal, but have something in common. We can aggregate results from both of them. For example, we can unify `indexFileCount` and `uploadedIndexFileCount` of incremental backup and `fileCount` of snapshot backup. Since snapshot is not incremental by its nature, we can fill both `indexFileCount` and `uploadedIndexFileCount` of aggregated result with `fileCount` value.
Here is an example of aggregated results of the snapshot backup response (before and after changes applied):
Before:
```json
"response": {
"collection": "techproducts",
"numShards": 2,
"endTime": "2022-02-09T07:25:52.879269500Z"
}
```
After:
```json
"response": {
"collection": "techproducts",
"numShards": 2,
"backupId": -1,
"indexVersion": "9.0.0",
"startTime": "2022-02-09T19:16:10.082015800Z",
"indexFileCount": 22,
"uploadedIndexFileCount": 22,
"endTime": "2022-02-09T19:16:11.191537400Z"
}
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1037068373
> Otherwise I think this is probably good to go once the docs are updated, there is a changelog entry for 9.0, and some upgrade notes in major-changes-in-solr-9.adoc, though that can be brief.
I've updated `major-changes-in-solr-9.adoc` to add notes about above changes. I added them to `Solr 9.0 Raw Notes` section. Maybe it should be extracted to separate topic? Feel free to update the text if needed:
* SOLR-15982: For collection's snapshot backup request responses additional fields `indexVersion`, `indexFileCount`, etc. were added similar to incremental backup request responses. Also, both snapshot and incremental backup request responses will now contain `starTime` and `endTime`. Here is an example of how collection's snapshot backup request responses will looks like:
Incremental:
```json
"response": {
"collection": "techproducts",
"numShards": 2,
"backupId": 0,
"indexVersion": "9.0.0",
"startTime": "2022-02-11T17:20:44.157305500Z",
"indexFileCount": 22,
"uploadedIndexFileCount": 22,
"indexSizeMB": 0.007,
"uploadedIndexFileMB": 0.007,
"shardBackupIds": [
"md_shard2_0",
"md_shard1_0"
],
"endTime": "2022-02-11T17:20:45.245534400Z"
}
```
Snapshot:
```json
"response": {
"collection": "techproducts",
"numShards": 2,
"indexVersion": "9.0.0",
"startTime": "2022-02-11T17:19:33.271461700Z",
"indexFileCount": 22,
"endTime": "2022-02-11T17:19:34.363859100Z"
}
```
Snapshot backup shard's response were updated to add fields `indexFileCount` and `endTime`, snapshot delete shard's response were updated to add fields `startTime` and `endTime`. Previous fields `fileCount`, `snapshotCompletedAt` and `snapshotDeletedAt` of backup and delete shard's responses are now deprecated and will be removed in future releases. All date/time fields of backup and delete related shard's responses have been updated to use `Instance` instead of `Date`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r801550139
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java
##########
@@ -138,6 +138,15 @@ public void call(ClusterState state, ZkNodeProps message, NamedList<Object> resu
backupMgr.writeBackupProperties(backupProperties);
+ // It can't be done within aggregateResults call
+ // since endTime is filled later
+ if(backupProperties != null) {
Review comment:
Hi @cpoerschke Thanks for the note! It can be `null`, in case of non incremental call it will be redirected to `copyIndexFiles(URI, String, ZkNodeProps, NamedList<Object>)`:
```java
public void call(ClusterState state, ZkNodeProps message, NamedList<Object> results) throws Exception {
...
try (BackupRepository repository = cc.newBackupRepository(repo)) {
...
if (incremental) {
incrementalCopyIndexFiles(backupUri, collectionName, message, results, backupProperties, backupMgr);
} else {
copyIndexFiles(backupUri, collectionName, message, results);
}
...
}
...
}
```
and within `copyIndexFiles` it will call `aggregateResults` without providing `backupManager` and `backupProps`:
```java
private void copyIndexFiles(URI backupPath, String collectionName, ZkNodeProps request, NamedList<Object> results) throws Exception {
...
NamedList<Object> aggRsp = aggregateResults(results, collectionName, slices, null, null);
...
}
```
Maybe we should investigate this and it is safe to provide `copyIndexFiles` with `backupManager` and `backupProps`?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r806052548
##########
File path: solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-9.adoc
##########
@@ -104,6 +104,44 @@ All the usages are replaced by BaseHttpSolrClient.RemoteSolrException and BaseHt
In previous versions there was a field returned in async backup status responses, `Response`. This has now been renamed to `msg`, to better fit other collections API responses.
The `response` field is now a map, containing information about the backup (`startTime`, `indexSizeMB`, `indexFileCount`, etc.).
+* SOLR-15982: For collection's snapshot backup request responses additional fields `indexVersion`, `indexFileCount`, etc. were added similar to incremental backup request responses. Also, both snapshot and incremental backup request responses will now contain `starTime` and `endTime`. Here is an example of how collection's snapshot backup request responses will looks like:
+
+Incremental:
+
+```json
+"response": {
+ "collection": "techproducts",
+ "numShards": 2,
+ "backupId": 0,
+ "indexVersion": "9.0.0",
+ "startTime": "2022-02-11T17:20:44.157305500Z",
+ "indexFileCount": 22,
+ "uploadedIndexFileCount": 22,
+ "indexSizeMB": 0.007,
+ "uploadedIndexFileMB": 0.007,
+ "shardBackupIds": [
+ "md_shard2_0",
+ "md_shard1_0"
+ ],
+ "endTime": "2022-02-11T17:20:45.245534400Z"
+}
+```
+
+Snapshot:
+
+```json
+"response": {
+ "collection": "techproducts",
+ "numShards": 2,
+ "indexVersion": "9.0.0",
+ "startTime": "2022-02-11T17:19:33.271461700Z",
+ "indexFileCount": 22,
+ "endTime": "2022-02-11T17:19:34.363859100Z"
+}
+```
Review comment:
I agree that this is too much for the major changes page. I think you can add it to the collections api entry if you want, and style it like this: https://solr.apache.org/guide/8_11/collections-api.html#examples-of-async-requests, but it's not a requirement for this PR.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r805893449
##########
File path: solr/core/src/java/org/apache/solr/handler/SnapShooter.java
##########
@@ -272,9 +273,11 @@ public void createSnapAsync(final int numberToKeep, Consumer<NamedList<?>> resul
solrCore.getDirectoryFactory().release(dir);
}
- details.add("fileCount", files.size());
+ details.add("fileCount", files.size()); // DEPRECATED: for removal, replaced with indexFileCount
+ details.add("indexFileCount", files.size());
details.add("status", "success");
- details.add("snapshotCompletedAt", new Date().toString());//bad; should be Instant.now().toString()
+ details.add("snapshotCompletedAt", Instant.now().toString()); // DEPRECATED: for removal, replaced with endTime
+ details.add("endTime", Instant.now().toString());
Review comment:
Yeah, sure let's extract to local variable. Concerning sunset time, not sure what period is usually applied. One major release?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1036534858
Nvm, I think I fixed it. I will push.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio edited a comment on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio edited a comment on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1034127420
@cpoerschke @HoustonPutman
> Maybe we should investigate this is it safe to provide copyIndexFiles with backupManager and backupProps?
I've updated signature of `copyIndexFiles` method for non incremental (snapshot) backup to accept `backupManager` and `backupProperties`. In turn, it will pass them both to `aggregateResults`. This way `aggregateResults` will always get non null `backupManager` and `backupProperties` values, so it will be safe to extract necessary data from it. First of all we will get `startTime` (before it was `null` for snapshot backups).
Second, let's compare shard responses of incremental and snapshot backups:
Incremental:
```json
"response": {
"startTime": "2022-02-04T12:35:52.304582700Z",
"indexFileCount": 21,
"uploadedIndexFileCount": 21,
"indexSizeMB": 0.006,
"uploadedIndexFileMB": 0.006,
"shard": "shard1",
"endTime": "2022-02-04T12:35:52.348963900Z",
"shardBackupId": "md_shard1_0"
}
```
Snapshot:
```json
"response": {
"startTime": "Wed Feb 09 07:25:51 UTC 2022",
"fileCount": 21,
"status": "success",
"snapshotCompletedAt": "Wed Feb 09 07:25:51 UTC 2022",
"snapshotName": "shard1",
"directoryName": "snapshot.shard1"
}
```
They are not equal, but have something in common. We can aggregate results from both of them. For example, we can unify `indexFileCount` and `uploadedIndexFileCount` of incremental backup and `fileCount` of snapshot backup. Since snapshot is not incremental by its nature, we can fill both `indexFileCount` and `uploadedIndexFileCount` of aggregated result with `fileCount` value.
Here is an example of aggregated results of the snapshot backup response (before and after changes applied):
Before:
```json
"response": {
"collection": "techproducts",
"numShards": 2,
"endTime": "2022-02-09T07:25:52.879269500Z"
}
```
After:
```json
"response": {
"collection": "techproducts",
"numShards": 2,
"backupId": -1,
"indexVersion": "9.0.0",
"startTime": "2022-02-09T19:16:10.082015800Z",
"indexFileCount": 22,
"uploadedIndexFileCount": 22,
"endTime": "2022-02-09T19:16:11.191537400Z"
}
```
Note that for snapshot backups value of `backupId` will be always `-1`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r805901949
##########
File path: solr/core/src/java/org/apache/solr/handler/SnapShooter.java
##########
@@ -272,9 +273,11 @@ public void createSnapAsync(final int numberToKeep, Consumer<NamedList<?>> resul
solrCore.getDirectoryFactory().release(dir);
}
- details.add("fileCount", files.size());
+ details.add("fileCount", files.size()); // DEPRECATED: for removal, replaced with indexFileCount
+ details.add("indexFileCount", files.size());
details.add("status", "success");
- details.add("snapshotCompletedAt", new Date().toString());//bad; should be Instant.now().toString()
+ details.add("snapshotCompletedAt", Instant.now().toString()); // DEPRECATED: for removal, replaced with endTime
+ details.add("endTime", Instant.now().toString());
Review comment:
> Maybe have a local to ensure both timestamps are 100% identical
Done
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1036372884
Hi, @cpoerschke ! Thank you for the comments!
> Could it also be confusing for users to have both indexFileCount and uploadedIndexFileCount values instead of the one fileCount value? I'm curious though why it's called fileCount and not indexFileCount in the snapshot i.e. would a snapshot after an incremental backup not have the same count for both (assuming no changes in the interim)?
Yes, I agree! After I have pushed changes I thought the same :) We can expose only `fileCount` single field for snapshot backups. It is not a problem. Even better, we can use `indexFileCount` field to exactly express the nature of the content. This way aggregated result for incremental backups will contain both `indexFileCount` and `uploadedIndexFileCount`, while snapshot backups will only contain `indexFileCount` field.
> Hmm, subjectively no backup id seems clearer than a backup id of -1 value, but that's just me.
OK. We can exclude it. Value `-1` is just a value that is assigned for non incremental backups within `BackupManager.forBackup` call:
```java
BackupManager backupMgr = (incremental) ?
BackupManager.forIncrementalBackup(repository, ccc.getZkStateReader(), backupUri) :
BackupManager.forBackup(repository, ccc.getZkStateReader(), backupUri);
```
> Not connected to any specific response element(s), do we know what backwards compatibility considerations apply here and/or any thoughts on how to communicate changes to users? E.g. additional elements likely would not break existing usage of the JSON but renaming or removal of existing elements might.
You are absolutely right! The most pleasant thing here we are not going to change any shard's responses format. We are going to extend aggregated result content for snapshot backups only. Initially it is quite poor:
```json
"response": {
"collection": "techproducts",
"numShards": 2,
"endTime": "2022-02-09T07:25:52.879269500Z"
}
```
so we can extend it any way we want without worrying about breaking some backward compatibility.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman edited a comment on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
HoustonPutman edited a comment on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1036382541
Beyond what Christine has said, I have a few questions:
- I believe `snapshotCompletedAt` is a new field, correct? If so why not name it `endTime` to match the incremental backup response?
- I notice that there is a lot of mixing of date formats. Since this is a major version, can we go ahead and start using the ISO date format for everything? I think I noticed this was documented somewhere when I was working on the previous ticket... Ok looking at it, there are comments a few places saying that it should be changed from `new Date().toString()` to `Instant.now().toString()`. That should do the trick, if we make sure to use that throughout the backup code.
Also lets be sure to update the example output in `solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc` to match the new output
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1036445656
This is how aggregated result is looks like after changes/suggestions we discussed above:
For incremental backup:
```json
"response": {
"collection": "techproducts",
"numShards": 2,
"backupId": 0,
"indexVersion": "9.0.0",
"startTime": "2022-02-11T17:20:44.157305500Z",
"indexFileCount": 22,
"uploadedIndexFileCount": 22,
"indexSizeMB": 0.007,
"uploadedIndexFileMB": 0.007,
"shardBackupIds": [
"md_shard2_0",
"md_shard1_0"
],
"endTime": "2022-02-11T17:20:45.245534400Z"
}
```
For snapshot backup:
```json
"response": {
"collection": "techproducts",
"numShards": 2,
"indexVersion": "9.0.0",
"startTime": "2022-02-11T17:19:33.271461700Z",
"indexFileCount": 22,
"endTime": "2022-02-11T17:19:34.363859100Z"
}
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1036383929
Ahh just realized that `snapshotCompletedAt` is actually not a new field. Maybe we add a duplicate field `endTime`, so that we can phase out `snapshotCompletedAt` in Solr 10?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r805760965
##########
File path: solr/core/src/java/org/apache/solr/handler/SnapShooter.java
##########
@@ -272,9 +273,11 @@ public void createSnapAsync(final int numberToKeep, Consumer<NamedList<?>> resul
solrCore.getDirectoryFactory().release(dir);
}
- details.add("fileCount", files.size());
+ details.add("fileCount", files.size()); // DEPRECATED: for removal, replaced with indexFileCount
+ details.add("indexFileCount", files.size());
details.add("status", "success");
- details.add("snapshotCompletedAt", new Date().toString());//bad; should be Instant.now().toString()
+ details.add("snapshotCompletedAt", Instant.now().toString()); // DEPRECATED: for removal, replaced with endTime
+ details.add("endTime", Instant.now().toString());
Review comment:
Maybe have a local to ensure both timestamps are 100% identical and/or indicate intended removal timeline in the comment e.g.
```suggestion
final String endTime = Instant.now();
details.add("snapshotCompletedAt", endTime); // DEPRECATED: for removal in Solr 10, replaced with endTime
details.add("endTime", endTime);
```
##########
File path: solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-9.adoc
##########
@@ -104,6 +104,44 @@ All the usages are replaced by BaseHttpSolrClient.RemoteSolrException and BaseHt
In previous versions there was a field returned in async backup status responses, `Response`. This has now been renamed to `msg`, to better fit other collections API responses.
The `response` field is now a map, containing information about the backup (`startTime`, `indexSizeMB`, `indexFileCount`, etc.).
+* SOLR-15982: For collection's snapshot backup request responses additional fields `indexVersion`, `indexFileCount`, etc. were added similar to incremental backup request responses. Also, both snapshot and incremental backup request responses will now contain `starTime` and `endTime`. Here is an example of how collection's snapshot backup request responses will looks like:
+
+Incremental:
+
+```json
+"response": {
+ "collection": "techproducts",
+ "numShards": 2,
+ "backupId": 0,
+ "indexVersion": "9.0.0",
+ "startTime": "2022-02-11T17:20:44.157305500Z",
+ "indexFileCount": 22,
+ "uploadedIndexFileCount": 22,
+ "indexSizeMB": 0.007,
+ "uploadedIndexFileMB": 0.007,
+ "shardBackupIds": [
+ "md_shard2_0",
+ "md_shard1_0"
+ ],
+ "endTime": "2022-02-11T17:20:45.245534400Z"
+}
+```
+
+Snapshot:
+
+```json
+"response": {
+ "collection": "techproducts",
+ "numShards": 2,
+ "indexVersion": "9.0.0",
+ "startTime": "2022-02-11T17:19:33.271461700Z",
+ "indexFileCount": 22,
+ "endTime": "2022-02-11T17:19:34.363859100Z"
+}
+```
Review comment:
Suggest to omit the example here and to update and/or extend the "Output Snippet" in https://github.com/apache/solr/blob/main/solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc#backup-status instead.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio edited a comment on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio edited a comment on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1042023456
@HoustonPutman Oh, I was going to cleanup/update docs today but realized you have already done it. Thanks!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r801556566
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java
##########
@@ -138,6 +138,15 @@ public void call(ClusterState state, ZkNodeProps message, NamedList<Object> resu
backupMgr.writeBackupProperties(backupProperties);
+ // It can't be done within aggregateResults call
+ // since endTime is filled later
+ if(backupProperties != null) {
+ // It is safe to extract results here
+ @SuppressWarnings("unchecked")
+ NamedList<Object> response = (NamedList<Object>) results.get("response");
+ response.add("endTime", backupProperties.getEndTime());
Review comment:
@cpoerschke Good point! My idea was that calling this block:
```java
if(backupProperties != null) {
// It is safe to extract results here
@SuppressWarnings("unchecked")
NamedList<Object> response = (NamedList<Object>) results.get("response");
response.add("endTime", backupProperties.getEndTime());
}
```
immediately follows after the call of `backupMgr.writeBackupProperties(backupProperties)`. It is where `endTime` is initiated/filled. I was supposed that it will be guaranteed to fill. In case of exception thrown within `writeBackupProperties` it will terminate the outer `call` method completely.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r804897545
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java
##########
@@ -289,7 +294,10 @@ private void incrementalCopyIndexFiles(URI backupUri, String collectionName, ZkN
NamedList<?> shardResp = (NamedList<?>)((NamedList<?>)shards.getVal(i)).get("response");
if (shardResp == null)
continue;
- Integer shardIndexFileCount = (Integer) shardResp.get("indexFileCount");
+ // indexFileCount is expected to be found in case of incremental backups
+ // fileCount is expected to be found in case of snapshot backups
+ Integer shardIndexFileCount = (Integer) Optional.<Object> ofNullable(shardResp.get("indexFileCount"))
+ .orElse(shardResp.get("fileCount"));
Review comment:
Thank you, @cpoerschke ! But it seems it is not relevant anymore, I've added field `indexFileCount` to duplicate field `fileCount` in shard snapshot response if you don't mind:
```java
details.add("fileCount", files.size()); // TODO: for removal, replaced with indexFileCount
details.add("indexFileCount", files.size());
...
details.add("snapshotCompletedAt", Instant.now().toString()); // TODO: for removal, replaced with endTime
details.add("endTime", Instant.now().toString());
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1036498290
> I see one in deleteNamedSnapshot() as well.
Updated
> Understood, but I think the best user experience is that people can expect the same field names for single-shard vs multi-shard. So might as well do it in this PR 🙂
Agree! I've added duplicate fields to shard response:
- `indexFileCount` for `fileCount`
- `endTime` for `snapshotCompletedAt`
```java
details.add("fileCount", files.size()); // TODO: for removal, replaced with indexFileCount
details.add("indexFileCount", files.size());
...
details.add("snapshotCompletedAt", Instant.now().toString()); // TODO: for removal, replaced with endTime
details.add("endTime", Instant.now().toString());
```
Here is an example of shard responses:
For incremental:
```json
"response": {
"startTime": "2022-02-11T18:07:34.306028400Z",
"indexFileCount": 21,
"uploadedIndexFileCount": 21,
"indexSizeMB": 0.006,
"uploadedIndexFileMB": 0.006,
"shard": "shard1",
"endTime": "2022-02-11T18:07:34.350180600Z",
"shardBackupId": "md_shard1_0"
}
```
For snapshot:
```json
"response": {
"startTime": "2022-02-11T18:08:31.405757700Z",
"fileCount": 21,
"indexFileCount": 21,
"status": "success",
"snapshotCompletedAt": "2022-02-11T18:08:31.437883800Z",
"endTime": "2022-02-11T18:08:31.437883800Z",
"snapshotName": "shard1",
"directoryName": "snapshot.shard1"
}
```
@cpoerschke how you think, is it OK?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1036502351
@HoustonPutman @cpoerschke
How you think, maybe we should do the same trick for delete snapshot also? I mean adding field `startTime` (it is not added for some reason) and adding field `endTime` to duplicate `snapshotDeletedAt`?
This is how `deleteNamedSnapshot` looks like right now:
```java
protected void deleteNamedSnapshot(ReplicationHandler replicationHandler) {
log.info("Deleting snapshot: {}", snapshotName);
NamedList<Object> details = new NamedList<>();
details.add("snapshotName", snapshotName);
try {
URI path = baseSnapDirPath.resolve("snapshot." + snapshotName);
backupRepo.deleteDirectory(path);
details.add("status", "success");
details.add("snapshotDeletedAt", Instant.now().toString());
} catch (IOException e) {
details.add("status", "Unable to delete snapshot: " + snapshotName);
log.warn("Unable to delete snapshot: {}", snapshotName, e);
}
replicationHandler.snapShootDetails = details;
}
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1048088655
@HoustonPutman @cpoerschke Thank you also for your work and guidance! :)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1036240452
> Second, let's compare shard responses of incremental and snapshot backups:
>
> Incremental:
>
> ```json
> "response": {
> "startTime": "2022-02-04T12:35:52.304582700Z",
> "indexFileCount": 21,
> "uploadedIndexFileCount": 21,
> "indexSizeMB": 0.006,
> "uploadedIndexFileMB": 0.006,
> "shard": "shard1",
> "endTime": "2022-02-04T12:35:52.348963900Z",
> "shardBackupId": "md_shard1_0"
> }
> ```
>
> Snapshot:
>
> ```json
> "response": {
> "startTime": "Wed Feb 09 07:25:51 UTC 2022",
> "fileCount": 21,
> "status": "success",
> "snapshotCompletedAt": "Wed Feb 09 07:25:51 UTC 2022",
> "snapshotName": "shard1",
> "directoryName": "snapshot.shard1"
> }
> ```
_Thanks for including the response examples, that really helps with understanding of the code and the code changes!_
> They are not equal, but have something in common. We can aggregate results from both of them. For example, we can unify `indexFileCount` and `uploadedIndexFileCount` of incremental backup and `fileCount` of snapshot backup. Since snapshot is not incremental by its nature, we can fill both `indexFileCount` and `uploadedIndexFileCount` of aggregated result with `fileCount` value.
_Could it also be confusing for users to have both `indexFileCount` and `uploadedIndexFileCount` values instead of the one `fileCount` value? I'm curious though why it's called `fileCount` and not `indexFileCount` in the snapshot i.e. would a snapshot after an incremental backup not have the same count for both (assuming no changes in the interim)?_
> Here is an example of aggregated results of the snapshot backup response (before and after changes applied):
>
> Before:
>
> ```json
> "response": {
> "collection": "techproducts",
> "numShards": 2,
> "endTime": "2022-02-09T07:25:52.879269500Z"
> }
> ```
>
> After:
>
> ```json
> "response": {
> "collection": "techproducts",
> "numShards": 2,
> "backupId": -1,
> "indexVersion": "9.0.0",
> "startTime": "2022-02-09T19:16:10.082015800Z",
> "indexFileCount": 22,
> "uploadedIndexFileCount": 22,
> "endTime": "2022-02-09T19:16:11.191537400Z"
> }
> ```
>
> Note that for snapshot backups value of `backupId` will be always `-1`
_Hmm, subjectively no backup id seems clearer than a backup id of `-1` value, but that's just me._
_Not connected to any specific response element(s), do we know what backwards compatibility considerations apply here and/or any thoughts on how to communicate changes to users? E.g. additional elements likely would not break existing usage of the JSON but renaming or removal of existing elements might._
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1036481824
> Good catch! I encountered this kind of comment at org.apache.solr.handler.SnapShooter.createSnapshot(IndexCommit). It seems this is the only place. Let's update it!
I see one in `deleteNamedSnapshot()` as well.
> Yeah, it is not a new field. We doesn't touch shard's responses, only updating aggregated result. I will check out shard response and I think I can add an additional fields endTime to duplicate snapshotCompletedAt.
Understood, but I think the best user experience is that people can expect the same field names for single-shard vs multi-shard. So might as well do it in this PR 🙂
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman merged pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
HoustonPutman merged pull request #595:
URL: https://github.com/apache/solr/pull/595
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on a change in pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #595:
URL: https://github.com/apache/solr/pull/595#discussion_r809435451
##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/BackupCmd.java
##########
@@ -116,6 +116,10 @@ public void call(ClusterState state, ZkNodeProps message, NamedList<Object> resu
break;
}
case CollectionAdminParams.NO_INDEX_BACKUP_STRATEGY: {
+ NamedList<Object> response = new SimpleOrderedMap<>();
+ response.add("collection", collectionName);
+ response.add("startTime", backupProperties.getStartTime());
+ results.add("response", response);
Review comment:
Me too, luckily there was a test that caught it!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] HoustonPutman commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1043430550
Thanks for the awesome work @ijioio !
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1036522198
OMG, it seems I messed up with conflict merge in Eclipse...
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] ijioio commented on pull request #595: SOLR-15982: Add end time value to backup response
Posted by GitBox <gi...@apache.org>.
ijioio commented on pull request #595:
URL: https://github.com/apache/solr/pull/595#issuecomment-1036465001
@HoustonPutman Thank you for the notes!
> I believe snapshotCompletedAt is a new field, correct? If so why not name it endTime to match the incremental backup response?
Yeah, it is not a new field. We doesn't touch shard's responses, only updating aggregated result. I will check out shard response and I think I can add an additional fields `endTime` to duplicate `snapshotCompletedAt`.
> I notice that there is a lot of mixing of date formats. Since this is a major version, can we go ahead and start using the ISO date format for everything? I think I noticed this was documented somewhere when I was working on the previous ticket... Ok looking at it, there are comments a few places saying that it should be changed from new Date().toString() to Instant.now().toString(). That should do the trick, if we make sure to use that throughout the backup code.
Good catch! I encountered this kind of comment at `org.apache.solr.handler.SnapShooter.createSnapshot(IndexCommit)`. It seems this is the only place. Let's update it!
> Also lets be sure to update the example output in solr/solr-ref-guide/modules/deployment-guide/pages/backup-restore.adoc to match the new output
Thanks for pointing this out! I totally forgot about documentation. If we will update shard's response of snapshot backup I will update the docs.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org