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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
