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.

Sign up for GitHub

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

Jump to bottom

[Downloader] Simplify downloader code #330

Merged
merged 2 commits into from
Oct 25, 2019
Merged

[Downloader] Simplify downloader code #330

merged 2 commits into from
Oct 25, 2019

Conversation

apeksharma
Copy link
Contributor

@apeksharma apeksharma commented Oct 17, 2019 •
edited
Loading

Motivation:
By abstracting out few functions, we can easily simplify some of the downloader code i.e. simplify functions' parameters, less places with switch-case branching logic, etc.

Other changes:
Call Utility.extractHashAndSigFromFile once instead of 3 times.

Which issue(s) this PR fixes:
Fixes #167 too.

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

@apeksharma apeksharma force-pushed the downloader_improvements_1 branch from 151a77b to afeab4d Compare October 17, 2019 22:20
@apeksharma apeksharma requested review from steven-sheehy and removed request for steven-sheehy October 17, 2019 22:21
@codecov
Copy link

codecov bot commented Oct 17, 2019 •
edited
Loading

Codecov Report

Merging #330 into master will decrease coverage by 0.18%.
The diff coverage is 72.13%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #330      +/-   ##
============================================
- Coverage     51.97%   51.79%   -0.19%     
+ Complexity      437      431       -6     
============================================
  Files            56       56              
  Lines          2738     2707      -31     
  Branches        373      366       -7     
============================================
- Hits           1423     1402      -21     
+ Misses         1151     1147       -4     
+ Partials        164      158       -6
Impacted Files Coverage Δ Complexity Δ
...or/downloader/event/EventStreamFileDownloader.java 4% <0%> (-0.06%) 2 <0> (ø)
...mirror/downloader/record/RecordFileDownloader.java 68.57% <100%> (+0.45%) 15 <1> (+1) ⬆️
.../downloader/balance/AccountBalancesDownloader.java 73.68% <100%> (+0.95%) 15 <1> (+1) ⬆️
src/main/java/com/hedera/utilities/Utility.java 57.09% <33.33%> (+0.14%) 64 <3> (-2) ⬇️
...edera/mirror/downloader/NodeSignatureVerifier.java 67.39% <75%> (-4.95%) 7 <4> (-2)
.../java/com/hedera/mirror/downloader/Downloader.java 85.34% <84.61%> (+4.09%) 14 <7> (-4) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e76ce9...051bf77. Read the comment docs.

apeksharma added a commit that referenced this pull request Oct 18, 2019
@apeksharma
[Downloader] Downloader improvements
b2b9f63 
This change uses DataTypeHandler interface introduced in last change (PR #330) to
replace 3 verifySigsAndDownload<Type>Files() fns by a common download function
(Downloader.verifySigsAndDownloadDataFiles()). Advantages: More code sharing,
removes multiple copies. Easier to maintain single logic rather than three
copies which keep differing with time.

(Note D = Downloader)
Notable changes:
- RecordD and eventsD didn't check for newFileName > oldFileName (since prevFileHash check was enough).
  The common function always checks for newFileName > oldFileName condition.
  It's needed for balances file and acts as extra validity check for record & event files.
- Earlier, eventsD was not sorting sigFiles before iterating on them, which led to more complicated
  veriffication logic.
  EventsD was first moving all files (after verifying sig) to valid dir and then checking for validity of
  prevFilehash in verifyValidFiles() fn. In contrast, recordD was checking for both, sigs and prevFileHash,
  before moving to vaild dir. And the logic was simpler and better too. So
  Downloader.verifySigsAndDownloadDataFiles() follows the latter one i.e. check sigs and hash-linking
  before moving to valid dir.
- In different download fns, checkStopFile() was sprinkled in different places. In new code, kept those ones
  around which are in tighter loops. The ones in new code seem okay to me to be valid exit points,
  I'd request reviewers to please take extra care to verify the same. Thanks.
- Unlike RecordsD, EventD wasn't respecting bypass parameter (EVENT_HASH_MISMATCH_BYPASS_UNTIL_AFTER).
  Now it does. If that's not the expected behavior, please let me know.

Signed-off-by: Apekshit Sharma <apekshit.sharma@hedera.com>
@apeksharma apeksharma mentioned this pull request Oct 18, 2019
Merged
2 tasks
@apeksharma
Copy link
Contributor Author

Here's the followup PR #331

@apeksharma apeksharma added this to the 0.4.0 milestone Oct 18, 2019
@apeksharma apeksharma added the downloader Area: S3 downloader label Oct 18, 2019
@apeksharma apeksharma force-pushed the downloader_improvements_1 branch from afeab4d to b88001d Compare October 23, 2019 00:16
@apeksharma apeksharma changed the title [Downloader] Simplify downloader code by encapsulating 'type specific' logic into separate class(DataTypeHandler) [Downloader] Simplify downloader code Oct 23, 2019
@apeksharma apeksharma force-pushed the downloader_improvements_1 branch 2 times, most recently from 9c9e2d8 to 0fb8764 Compare October 23, 2019 23:27
@apeksharma
[Downloader] Simplify downloader code
e0b292d 
Motivation:
By abstracting out few functions, we can easily simplify some of the downloader code i.e. share more code,
simplify functions' parameters, less places with switch-case branching logic, etc.

Other changes:

Call Utility.extractHashAndSigFromFile once instead of 3 times.

Which issue(s) this PR fixes:
Fixes #167 too.

Signed-off-by: Apekshit Sharma <apekshit.sharma@hedera.com>
@apeksharma apeksharma force-pushed the downloader_improvements_1 branch from 0fb8764 to e0b292d Compare October 23, 2019 23:28
apeksharma added a commit that referenced this pull request Oct 24, 2019
@apeksharma
[Downloader] Common sig verifier and data download code
96df0fd 
This change builds on the last change (PR #330) to replace 3
verifySigsAndDownload<Type>Files() fns by a single function
Downloader.verifySigsAndDownloadDataFiles(), therefore removing
3 copies of mostly similar code with one. This will make further
changes to Downloader code easier.

(Note D = Downloader)
Notable changes:
- RecordD and eventsD didn't check for newFileName > oldFileName (since prevFileHash check was enough).
  The common function always checks for newFileName > oldFileName condition.
  It's needed for balances file and acts as extra validity check for record & event files.
- Earlier, eventsD was not sorting sigFiles before iterating on them, which led to more complicated
  verification logic.
  EventsD was first moving all files (after verifying sig) to valid dir and then checking for validity of
  prevFilehash in verifyValidFiles() fn. In contrast, recordD was checking for both, sigs and prevFileHash,
  before moving to vaild dir. And the logic was simpler and better too. So
  Downloader.verifySigsAndDownloadDataFiles() follows the latter one i.e. check sigs and hash-linking
  before moving to valid dir.
- Unlike RecordsD, EventD wasn't respecting bypass parameter (EVENT_HASH_MISMATCH_BYPASS_UNTIL_AFTER).
  Now it does. If that's not the expected behavior, please let me know.

Signed-off-by: Apekshit Sharma <apekshit.sharma@hedera.com>
steven-sheehy
steven-sheehy requested changes Oct 24, 2019
View reviewed changes
src/main/java/com/hedera/mirror/downloader/Downloader.java Outdated Show resolved Hide resolved
src/main/java/com/hedera/mirror/downloader/balance/AccountBalancesDownloader.java Outdated Show resolved Hide resolved
src/main/java/com/hedera/mirror/downloader/event/EventDownloaderProperties.java Outdated Show resolved Hide resolved
src/main/java/com/hedera/mirror/downloader/balance/BalanceDownloaderProperties.java Outdated Show resolved Hide resolved
src/main/java/com/hedera/mirror/downloader/record/RecordDownloaderProperties.java Outdated Show resolved Hide resolved
src/main/java/com/hedera/mirror/downloader/DownloaderProperties.java Outdated Show resolved Hide resolved
src/main/java/com/hedera/mirror/downloader/balance/AccountBalancesDownloader.java Outdated Show resolved Hide resolved
src/main/java/com/hedera/mirror/downloader/NodeSignatureVerifier.java Outdated Show resolved Hide resolved
@apeksharma apeksharma force-pushed the downloader_improvements_1 branch from b07d434 to 38f2c62 Compare October 24, 2019 23:28
@apeksharma
Address review comments
051bf77 
Signed-off-by: Apekshit Sharma <apekshit.sharma@hedera.com>
@apeksharma apeksharma force-pushed the downloader_improvements_1 branch from 38f2c62 to 051bf77 Compare October 25, 2019 00:08
steven-sheehy
steven-sheehy approved these changes Oct 25, 2019
View reviewed changes
Copy link
Member

@steven-sheehy steven-sheehy left a comment

Choose a reason for hiding this comment

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

Looks good. Much cleaner downloader code now.

@steven-sheehy steven-sheehy merged commit 810eb29 into master Oct 25, 2019
@steven-sheehy steven-sheehy deleted the downloader_improvements_1 branch October 25, 2019 03:14
apeksharma added a commit that referenced this pull request Oct 25, 2019
@apeksharma
[Downloader] Common sig verifier and data download code
24207d6 
This change builds on the last change (PR #330) to replace 3
verifySigsAndDownload<Type>Files() fns by a single function
Downloader.verifySigsAndDownloadDataFiles(), therefore removing
3 copies of mostly similar code with one. This will make further
changes to Downloader code easier.

(Note D = Downloader)
Notable changes:
- RecordD and eventsD didn't check for newFileName > oldFileName (since prevFileHash check was enough).
  The common function always checks for newFileName > oldFileName condition.
  It's needed for balances file and acts as extra validity check for record & event files.
- Earlier, eventsD was not sorting sigFiles before iterating on them, which led to more complicated
  verification logic.
  EventsD was first moving all files (after verifying sig) to valid dir and then checking for validity of
  prevFilehash in verifyValidFiles() fn. In contrast, recordD was checking for both, sigs and prevFileHash,
  before moving to vaild dir. And the logic was simpler and better too. So
  Downloader.verifySigsAndDownloadDataFiles() follows the latter one i.e. check sigs and hash-linking
  before moving to valid dir.
- Unlike RecordsD, EventD wasn't respecting bypass parameter (EVENT_HASH_MISMATCH_BYPASS_UNTIL_AFTER).
  Now it does. If that's not the expected behavior, please let me know.

Signed-off-by: Apekshit Sharma <apekshit.sharma@hedera.com>
@apeksharma apeksharma mentioned this pull request Oct 25, 2019
Merged
2 tasks
@apeksharma
Copy link
Contributor Author

Thanks for the review and merging.

apeksharma added a commit that referenced this pull request Nov 1, 2019
@apeksharma
[Downloader] Common sig verifier and data download code
85cd67f 
This change builds on the last change (PR #330) to replace 3
verifySigsAndDownload<Type>Files() fns by a single function
Downloader.verifySigsAndDownloadDataFiles(), therefore removing
3 copies of mostly similar code with one. This will make further
changes to Downloader code easier.

(Note D = Downloader)
Notable changes:
- RecordD and eventsD didn't check for newFileName > oldFileName (since prevFileHash check was enough).
  The common function always checks for newFileName > oldFileName condition.
  It's needed for balances file and acts as extra validity check for record & event files.
- Earlier, eventsD was not sorting sigFiles before iterating on them, which led to more complicated
  verification logic.
  EventsD was first moving all files (after verifying sig) to valid dir and then checking for validity of
  prevFilehash in verifyValidFiles() fn. In contrast, recordD was checking for both, sigs and prevFileHash,
  before moving to vaild dir. And the logic was simpler and better too. So
  Downloader.verifySigsAndDownloadDataFiles() follows the latter one i.e. check sigs and hash-linking
  before moving to valid dir.
- Unlike RecordsD, EventD wasn't respecting bypass parameter (EVENT_HASH_MISMATCH_BYPASS_UNTIL_AFTER).
  Now it does. If that's not the expected behavior, please let me know.

Signed-off-by: Apekshit Sharma <apekshit.sharma@hedera.com>
steven-sheehy pushed a commit that referenced this pull request Nov 1, 2019
@apeksharma @steven-sheehy
Refactor Downloader (#341)
c0df6d6 
* [Downloader] Common sig verifier and data download code

This change builds on the last change (PR #330) to replace 3
verifySigsAndDownload<Type>Files() fns by a single function
Downloader.verifySigsAndDownloadDataFiles(), therefore removing
3 copies of mostly similar code with one. This will make further
changes to Downloader code easier.

(Note D = Downloader)
Notable changes:
- RecordD and eventsD didn't check for newFileName > oldFileName (since prevFileHash check was enough).
  The common function always checks for newFileName > oldFileName condition.
  It's needed for balances file and acts as extra validity check for record & event files.
- Earlier, eventsD was not sorting sigFiles before iterating on them, which led to more complicated
  verification logic.
  EventsD was first moving all files (after verifying sig) to valid dir and then checking for validity of
  prevFilehash in verifyValidFiles() fn. In contrast, recordD was checking for both, sigs and prevFileHash,
  before moving to vaild dir. And the logic was simpler and better too. So
  Downloader.verifySigsAndDownloadDataFiles() follows the latter one i.e. check sigs and hash-linking
  before moving to valid dir.
- Unlike RecordsD, EventD wasn't respecting bypass parameter (EVENT_HASH_MISMATCH_BYPASS_UNTIL_AFTER).
  Now it does. If that's not the expected behavior, please let me know.

Signed-off-by: Apekshit Sharma <apekshit.sharma@hedera.com>

* Remove unsued imports

Signed-off-by: Apekshit Sharma <apekshit.sharma@hedera.com>

* Address review comments.

Signed-off-by: Apekshit Sharma <apekshit.sharma@hedera.com>

* use Answers.RETURNS_SMART_NULLS

Signed-off-by: Apekshit Sharma <apekshit.sharma@hedera.com>

* address review comments

Signed-off-by: Apekshit Sharma <apekshit.sharma@hedera.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steven-sheehy steven-sheehy steven-sheehy approved these changes

@gregscullard gregscullard Awaiting requested review from gregscullard

Assignees

@gregscullard gregscullard

@steven-sheehy steven-sheehy

Labels
downloader Area: S3 downloader enhancement Type: New feature P3 technical debt
Projects
None yet
Milestone
Mirror 0.4.0
Development

Successfully merging this pull request may close these issues.

Hash extracted twice
3 participants
@apeksharma @steven-sheehy @gregscullard