Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Harvester improvements. #6087

Merged
merged 4 commits into from
Aug 16, 2019
Merged

Harvester improvements. #6087

merged 4 commits into from
Aug 16, 2019

Conversation

landreev
Copy link
Contributor

fixes the issue with transaction scope when deleting harvested records, per OAI instructions;
plus some cosmetic issues and minor optimizations. (#3268)

New Contributors

Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Thanks!

Related Issues

Pull Request Checklist

fixes the issue with transaction scope when deleting harvested records, per OAI instructions;
plus some cosmetic issues and minor optimizations. (#3268)
@landreev landreev removed their assignment Aug 13, 2019
@sekmiller sekmiller assigned sekmiller and unassigned sekmiller Aug 14, 2019
@dataversebot
Copy link

Can one of the admins verify this patch?

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine so I'm approving it but I did leave a couple comments about things I'm curious about.

public void deleteHarvestedDataset(Dataset dataset, DataverseRequest request, Logger hdLogger) {
// Purge all the SOLR documents associated with this client from the
// index server:
indexService.deleteHarvestedDocuments(dataset);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this indexing operation be moved to "on success"?

@@ -315,8 +309,7 @@ public void doHarvest(DataverseRequest dataverseRequest, Long harvestingClientId



@TransactionAttribute(TransactionAttributeType.NOT_SUPPORTED)
public Long processRecord(DataverseRequest dataverseRequest, Logger hdLogger, PrintWriter importCleanupLog, OaiHandler oaiHandler, String identifier, MutableBoolean recordErrorOccurred, MutableLong processedSizeThisBatch, List<String> deletedIdentifiers) {
private Long processRecord(DataverseRequest dataverseRequest, Logger hdLogger, PrintWriter importCleanupLog, OaiHandler oaiHandler, String identifier, MutableBoolean recordErrorOccurred, MutableLong processedSizeThisBatch, List<String> deletedIdentifiers, Date dateStamp) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why "TransactionAttributeType.NOT_SUPPORTED" has been removed. I'm not saying this is a problem, I'm just a little turned around about why this change is being made.

@kcondon kcondon merged commit a1a5733 into develop Aug 16, 2019
@kcondon kcondon deleted the 3268-harvester-improvements branch August 16, 2019 20:49
@kcondon kcondon self-assigned this Aug 19, 2019
@djbrooke djbrooke added this to the 4.16 milestone Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix real-time, incremental harvesting from Odum archive
7 participants