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
Switch to SBC in KFF (incl. HashPlace) #368
base: main
Are you sure you want to change the base?
Switch to SBC in KFF (incl. HashPlace) #368
Conversation
We are going to need to see the performance impact of this at full scale. My gut feeling is that using a SBC here will be wasteful and impact throughput. |
5187a60
to
4a1693e
Compare
PR#204 ("Large data support") only calculated the hashes using the SeekableByteChannel. PR#204 spent months being tested on the development system with no observable performance impact by either myself or anyone else on the team. It was not until PR#362 ("KFFHashPlace should prefer byte[] to SeekableByteChannelFactory") that this behavior was changed to calculating hashes using the byte[]. Therefore, I believe the performance impact of exclusively calculating the hashes using the SeekableByteChannel has already been shown to be negligible. |
4a1693e
to
c8616a9
Compare
@jdcove2 I hear your points but it has not been tested at full scale. You quote no observable performance differences. Do you have any data to back up those claims? But as a side note. Emissary is a configuration-driven framework. Why should users of emissary be forced to use SBC if they don't want to? I will defer to senior devs to make the decisions on this going forward. |
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.
Just a couple minor changes suggested. None are blockers.
src/main/java/emissary/core/channels/SeekableByteChannelHelper.java
Outdated
Show resolved
Hide resolved
Given drivenflywheel's comment in PR#362 that "PRs work great for discussion, and they leave a much better historical record than a verbal discussion", it seems that this is the appropriate place to discuss your above concerns. Therefore, I have the following questions...
|
Thanks for the extra details Dave. As for the full scale I personally would have liked to see a statistical analysis of the performance on a full scale system. That can be interpolated from a smaller system with a different resource profile. But it appears that the team is ok accepting this how it is. And not allowing for it to be configurable so I will accept that moving forward. |
From my questions above...
|
c4af020
to
521ae39
Compare
521ae39
to
7b695da
Compare
7b695da
to
9ec9a5f
Compare
944d558
to
ef575a3
Compare
ef575a3
to
ea8a568
Compare
ea8a568
to
e25a082
Compare
@ukengineer925 @jdcove2 This might require some rollback of previous work but a more acceptable and easier to integrate approach would be to have a new KFF hash place or handler that uses sbc. And maybe something changes later in time and it is realized that the old non sbc KFF hash place is never used and not needed then it can go through the deprecation and removal process. |
@cfkoehler - I've added a convenience method to allow users to pass in a byte array of data if they really don't want to create their own SBCF (though as you'll see, there's a neat helper method to do that). I don't think maintaining two copies of KffDataObjectHandler, or two paths within one class, is optimal hence this PR. |
Removes complexity within HashPlace/KffDataObjectHandler to just use the SBC path. Adds additional tests to confirm behaviour and functionality.