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

Reverted KffFile back to using RandomAccessFile for file I/O, but now synchronizing access to its use #605

Merged
merged 3 commits into from
Oct 11, 2023
Merged

Reverted KffFile back to using RandomAccessFile for file I/O, but now synchronizing access to its use #605

merged 3 commits into from
Oct 11, 2023

Conversation

drivenflywheel
Copy link
Collaborator

Verified that KffFileTest.testConcurrentKffFileCheckCalls() passes with synchronization in place, fails with the synchronization disabled

@cfkoehler cfkoehler self-requested a review October 5, 2023 01:30
cfkoehler
cfkoehler previously approved these changes Oct 5, 2023
View reviewed changes
Copy link
Collaborator

@cfkoehler cfkoehler left a comment

Choose a reason for hiding this comment

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

I might not be the best review on this.
But I am able to understand the changes and it performs as I would expect.

@jpdahlke jpdahlke added this to the v8.0.0-M8 milestone Oct 5, 2023
@jpdahlke jpdahlke added the priority This has mirrored work driving the need label Oct 6, 2023
@jpdahlke jpdahlke requested a review from jdcove2 October 6, 2023 15:34
jdcove2
jdcove2 requested changes Oct 10, 2023
View reviewed changes
Copy link
Collaborator

@jdcove2 jdcove2 left a comment

Choose a reason for hiding this comment

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

Just as an FYI ... the calculations in this class use integers instead of longs, so there is an implied assumption that the file is no bigger than 2^31.

src/main/java/emissary/kff/KffFile.java Outdated Show resolved Hide resolved
src/main/java/emissary/kff/KffFile.java Outdated Show resolved Hide resolved
@drivenflywheel drivenflywheel force-pushed the KffFile_Synchronized_RandomAccessFile branch from 9fb3be2 to bc26bde Compare October 10, 2023 18:46
@drivenflywheel
KffFile: replaced knownFile.read(..byte[]) with knownFile.readFully(b…
22ece71 
…yte[]) to prevent bailing on incomplete reads.
@drivenflywheel drivenflywheel force-pushed the KffFile_Synchronized_RandomAccessFile branch from bc26bde to 22ece71 Compare October 10, 2023 18:49
jdcove2
jdcove2 previously approved these changes Oct 10, 2023
View reviewed changes
@jpdahlke jpdahlke merged commit 6a0b8cc into NationalSecurityAgency:master Oct 11, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfkoehler cfkoehler cfkoehler approved these changes

@jdcove2 jdcove2 jdcove2 approved these changes

Assignees
No one assigned
Labels
priority This has mirrored work driving the need
Projects
None yet
Milestone
v8.0.0-M8
Development

Successfully merging this pull request may close these issues.
4 participants
@drivenflywheel @jdcove2 @cfkoehler @jpdahlke