-
Notifications
You must be signed in to change notification settings - Fork 293
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
Add support for compressions and make LZ4 default for backup 2.0 #833
base: 3.x
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## 3.x #833 +/- ##
============================================
+ Coverage 46.56% 47.01% +0.45%
- Complexity 1052 1063 +11
============================================
Files 166 169 +3
Lines 7280 7393 +113
Branches 746 760 +14
============================================
+ Hits 3390 3476 +86
- Misses 3638 3660 +22
- Partials 252 257 +5
Continue to review full report at Codecov.
|
* this to sometimes C* creating files which are zero bytes, and giving that in | ||
* snapshot for some unknown reason. | ||
*/ | ||
if (chunk.length > 0) rateLimiter.acquire(chunk.length); |
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.
I assume we still want to proceed with the upload of this 0 byte file to keep our s3 snapshots consistent with C* snapshot view, and hence we do not skip the rest of the upload code path even if the file has 0 byte - is this correct?
|
||
// Cache hit. Return the value. | ||
if (cacheResult != null) return cacheResult; | ||
if (lastUpdatedTimestamp != null) return true; |
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.
since we do not really check the value of the timestamp, what difference does this make relative to having a Boolean? Maybe I will discover its importance in other fails, if I do, will delete this comment.
import java.time.temporal.ChronoUnit; | ||
import javax.crypto.KeyGenerator; | ||
|
||
public class s3test { |
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.
This class has a lot of commented code. Any reason for keeping the commented code?
We have supported Snappy as the only compression type while uploading the files. We wanted to support customizable compression types and thus support for LZ4 and no compression.
In this PR we make LZ4 as the default compression type for backup 2.0
Future work: Do not compress Data.db which are already compressed.
In this PR we have encapsulated the directives on how to upload or download a file to class UploadDownloadDirectives. This includes directives like compression, encryption, retries to use etc.