test: Files Historic Plugin Unit Tests Pt. 3 #1019
Conversation
ata-nas
added
the
pull request
| // Seems that if we use Zstd.compress(data, DEFAULT_ZSTD_COMPRESSION_LEVEL); | ||
| // directly it will not produce the same result as wrapping | ||
| // a stream with the same compression level. For consistency, | ||
| // we need to use the same approach as the one used in the | ||
| // wrapStream method. This method is a convenience for | ||
| // compressing data in memory. | ||
| try (final ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| final OutputStream wrappedBaos = wrapStream(baos)) { | ||
| wrappedBaos.write(data); | ||
| wrappedBaos.close(); | ||
| // close before return so that we are absolutely sure that | ||
| // all data is written including possible footer and others | ||
| yield baos.toByteArray(); | ||
| } catch (final IOException e) { | ||
| throw new UncheckedIOException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
I have introduced this change for consistency reasons. It seems that using the static method produces slightly different output than a wrapped stream would, even if using the same compression level. Is it ok to do this? Is there a better way?
cc: @jasperpotts @jsync-swirlds
There was a problem hiding this comment.
Update: after a discussion, let's revert this. The difference in bytes is probably because the static method call has more context than the streaming approach and this means that it can do a better job at compressing. This will produce different bytes at the end of the day. As far as the consistency concern, we should probably not take it into account as the library should be non deterministic, based on multiple factors. The proper way to assert would be to always uncompress the data and then juxtapose that against the expected.
| @@ -133,7 +139,7 @@ public BlockAccessor blockAccessor(long blockNumber) { | |||
| * | |||
| * @return the minimum block number, or -1 if no block files are found | |||
| */ | |||
| public long minStoredBlockNumber() { | |||
| long minStoredBlockNumber() { | |||
There was a problem hiding this comment.
Minor bug on line 163 (GH did not allow me to include int in the preview as I have not made changes there)
...
L:163 return Long.parseLong(fileName.substring(0, fileName.indexOf('.')));
...(in the max method is the same). We are neglecting the "s" appended at the back of the filename (e.g. "10s.zip") which produces a number format exception.
- Solution 1: handle with fileName.indexOf('s')
- Solution 2: change the BlockPath resolution logic to exclude the "s"
Any opinions/preferences?
cc: @jsync-swirlds @jasperpotts @AlfredoG87 @Nana-EC @georgi-l95
There was a problem hiding this comment.
I would remove the s. I'd also remove the extra 0 characters and just have a single digit filename (regardless of grouping). We really shouldn't make reading these files manually "easy"; we don't want operators meddling in the filesystem, so why make it easier?
There was a problem hiding this comment.
Update: after a discussion, let's proceed with solution 1 for now, revisiting this later if we need to.
There was a problem hiding this comment.
#1027 is created for a future revisit if we decide.
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
…the compress convenience method for the zstd compression type for consistency + tests Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
Signed-off-by: Atanas Atanasov <[email protected]>
…ModuleInfo task) Signed-off-by: Atanas Atanasov <[email protected]>
…the end of zip file names Signed-off-by: Atanas Atanasov <[email protected]>
jasperpotts
force-pushed
the
1009-files-historic-plugin-unit-tests-pt-3
branch
from
eaa5b75 to
dd6c2be
Compare
Codecov ReportAttention: Patch coverage is @@ Coverage Diff @@
## 505-plugins-services #1019 +/- ##
==========================================================
+ Coverage 77.30% 79.56% +2.26%
- Complexity 737 748 +11
==========================================================
Files 99 98 -1
Lines 3256 3245 -11
Branches 316 312 -4
==========================================================
+ Hits 2517 2582 +65
+ Misses 638 550 -88
- Partials 101 113 +12
... and 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Reviewer Notes
Related Issue(s)
Closes #1009