Skip to content

Conversation

@romayalon
Copy link
Contributor

Signed-off-by: Romy romy2232@gmail.com

Explain the changes

  1. When compression is disabled pick the original size of the chunk instead of the compressed size.

Issues: Fixed #xxx / Gap #xxx

  1. Fixed BZ #1842254 (comment 4)

Testing Instructions:

Copy link
Member

@guymguym guymguym left a comment

Choose a reason for hiding this comment

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

@romayalon Notice that config.CHUNK_CODER_COMPRESS_TYPE is only a default value, but each chunk actually refers to a chunk_coder (a DB collection) where we define the compress_type configuration which applies to that chunk. This is meant to provide flexibility to have chunks with different compression types in the same system or bucket (for example disable compression for already-compressed media files, or switch to another compression).

So I think we should fix it in map_aggregate_chunks() to use size if compress_size is unset - this makes sense because when a chunk used "no" compression then freely speaking the compressed size is the same as size, it's just that we didn't store it in the DB. This fix should keep everything else intact and also account compress_size correctly for when we disable compression altogether.

WDYT?

function map_aggregate_chunks() {
    const compress_size = this.compress_size || this.size;
    emit(['', 'compress_size'], compress_size);
    emit([this.bucket, 'compress_size'], compress_size);
}

@romayalon romayalon force-pushed the romy-data-optimization branch 2 times, most recently from 7249745 to 9037d75 Compare October 5, 2020 07:12
@romayalon
Copy link
Contributor Author

@guymguym agreed and fixed.

Signed-off-by: Romy <romy2232@gmail.com>
@romayalon romayalon force-pushed the romy-data-optimization branch from 9037d75 to 2b55904 Compare October 5, 2020 09:41
@romayalon romayalon requested a review from guymguym October 5, 2020 11:25
@nimrod-becker nimrod-becker merged commit befc16f into noobaa:master Oct 5, 2020
@jeniawhite
Copy link
Contributor

@guymguym @nimrod-becker @romayalon
This needs to be updated in PostgreSQL PL/PGSQL as well.

*/
function map_aggregate_chunks() {
const compress_size = this.compress_size || this.size;
emit(['', 'compress_size'], this.compress_size);
Copy link
Member

Choose a reason for hiding this comment

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

@romayalon use compress_size here too

Copy link
Member

Choose a reason for hiding this comment

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

and not this.compress_size...
FYI @nimrod-becker

Copy link
Contributor

Choose a reason for hiding this comment

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

ah!

@guymguym
Copy link
Member

guymguym commented Oct 5, 2020

@jeniawhite Right.
Can you point out where?
Best if you can add a comment with a permalink to the source.

@jeniawhite
Copy link
Contributor

jeniawhite commented Oct 5, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants