-
Notifications
You must be signed in to change notification settings - Fork 115
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
Remove fk_rec_file_id from t_transactions #600
Conversation
- fk_rec_file_id in t_transactions was deprecated and its value is always 0. Removing the column. - No need of 2-part insert using f_file_create/complete. Just checking for presense on file start and inserting row on file complete is sufficient. - Earlier, fk_rec_file_id was foreign key, so many tests for transactions needed to added a row to t_record_files. That foreign key was dropped recently, so update tests to remove chaff. - Changes in REST code are limited to tests BRD uses t_record_files table to simulate blocks internally. So we can't just remove it. We have two possible options going forward: 1. Remove t_record_files tables. We have no regular need of finding which file a txn belongs to. If needed for debugging, one can use txn's epoch datetime and do prefix ls on S3. Will need to understand how BRD uses t_record_files and if it is just as easy to simulate blocks using t_transactions. 2. If we keep the table around, to support lookup of all txns associated with a file, we can add two columns filled by consensus timestamps of first and last transaction in the file. Signed-off-by: Apekshit Sharma <[email protected]>
Signed-off-by: Apekshit Sharma <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f_file_complete and f_file_create are no longer used and should be removed in a db migration.
hedera-mirror-importer/src/main/java/com/hedera/mirror/importer/domain/RecordFile.java
Outdated
Show resolved
Hide resolved
hedera-mirror-importer/src/main/java/com/hedera/mirror/importer/domain/Transaction.java
Show resolved
Hide resolved
hedera-mirror-importer/src/test/resources/application-default.yml
Outdated
Show resolved
Hide resolved
hedera-mirror-importer/src/main/java/com/hedera/mirror/importer/parser/StreamFileListener.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Apekshit Sharma <[email protected]>
Signed-off-by: Apekshit Sharma <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #600 +/- ##
============================================
- Coverage 64.79% 64.64% -0.16%
Complexity 171 171
============================================
Files 101 102 +1
Lines 3159 3142 -17
Branches 362 360 -2
============================================
- Hits 2047 2031 -16
Misses 959 959
+ Partials 153 152 -1
Continue to review full report at Codecov.
|
Signed-off-by: Apekshit Sharma <[email protected]>
Signed-off-by: Apekshit Sharma <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Will reapply once you've merged in Nana's REST changes, which might be a bit tricky
@apeksharma just pull in my changes, over yours for the rest side. I already took out the record logic after your last comment. Let me know if you're uncertain about any line |
updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Detailed description:
Just checking for presense on file start and inserting row on file complete is sufficient.
a row to t_record_files. That foreign key was dropped recently, so update tests to remove chaff.
BRD uses t_record_files table to simulate blocks internally. So we can't just remove it.
We have two possible options going forward:
If needed for debugging, one can use txn's epoch datetime and do prefix ls on S3.
Will need to understand how BRD uses t_record_files and if it is just as easy to simulate blocks
using t_transactions.
two columns filled by consensus timestamps of first and last transaction in the file.
Signed-off-by: Apekshit Sharma [email protected]
Which issue(s) this PR fixes:
Followup of #566
Special notes for your reviewer:
Followup from #585 (comment)
Checklist