Page MenuHomeSoftware Heritage
Differential D5419

Cassandra: Deduplicate lists passed to *_add endpoints
ClosedPublic
Actions

Authored by KShivendu on Apr 5 2021, 1:45 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Commits
rDSTOc96942b40648: Cassandra: Deduplicate lists passed to *_add endpoints
Summary

Related T2316

Previously only release_add supported deduplication.
This commit aligns other _add endpoints with it

Diff Detail

Repository
rDSTO Storage manager
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20637
Build 32027: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 32026: arc lint + arc unit

Event Timeline

KShivendu created this revision.Apr 5 2021, 1:45 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptApr 5 2021, 1:45 PM
swh-public-ci added a comment.Apr 5 2021, 1:51 PM

Build has FAILED

Patch application report for D5419 (id=19373)

Rebasing onto 0a270d1a7a...

First, rewinding head to replay your work on top of it...
Applying: Cassandra: Deduplicate lists passed to *_add endpoints
Changes applied before test
commit 1da153ea6881808224a5dd6cbf7b51314903c98f
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Mon Apr 5 17:09:33 2021 +0530

    Cassandra: Deduplicate lists passed to *_add endpoints
    
    Previously only release_add supported deduplication.
    This commit aligns other _add endpoints with it

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/1243/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/1243/console

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 5 2021, 1:51 PM
Harbormaster failed remote builds in B20430: Diff 19373!
vlorentz added a subscriber: vlorentz.EditedApr 6 2021, 1:50 PM

I don't think you need to convert the sets back to lists

swh-public-ci added a comment.Apr 6 2021, 1:56 PM

Build has FAILED

Patch application report for D5419 (id=19373)

Rebasing onto 0a270d1a7a...

First, rewinding head to replay your work on top of it...
Applying: Cassandra: Deduplicate lists passed to *_add endpoints
Changes applied before test
commit 59383a65f4b85bb87a6d22f67635140de6982230
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Mon Apr 5 17:09:33 2021 +0530

    Cassandra: Deduplicate lists passed to *_add endpoints
    
    Previously only release_add supported deduplication.
    This commit aligns other _add endpoints with it

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/1244/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/1244/console

KShivendu requested review of this revision.Apr 7 2021, 1:42 PM

I don't think you need to convert the sets back to lists

I did that and got type errors from mypy.
Imo, it's okay to leave it as list(set(..)) because anyhow it gets transformed back into a list within the next 2-3 lines.
What do you think?

vlorentz added a comment.Apr 7 2021, 1:56 PM

I did that and got type errors from mypy.

Then only keep list() when needed. For example, it isn't needed in content_add or snapshot_add

Imo, it's okay to leave it as list(set(..)) because anyhow it gets transformed back into a list within the next 2-3 lines.

But it creates a copy that we don't need to do

KShivendu added a comment.Apr 7 2021, 2:11 PM

I just discovered that tests failed because the set's internally used hash function throws an error for if a dictionary is passed.
Do you know any other trick which can do the de-duplication in one line? Or should I just create a common function to loop over the list and find the unique ones?

vlorentz added a comment.EditedApr 7 2021, 2:13 PM

These objects has an id attribute. You can use it for deduplication (eg. via a dict)

KShivendu updated this revision to Diff 19459.Apr 7 2021, 4:12 PM

Updating D5419: Cassandra: Deduplicate lists passed to *_add endpoints

swh-public-ci added a comment.Apr 7 2021, 4:18 PM

Build has FAILED

Patch application report for D5419 (id=19459)

Rebasing onto 39507b24d0...

First, rewinding head to replay your work on top of it...
Applying: Cassandra: Deduplicate lists passed to *_add endpoints
Changes applied before test
commit 3526f21c81e4d0f8feeb09a88485d1e9790b03c3
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Mon Apr 5 17:09:33 2021 +0530

    Cassandra: Deduplicate lists passed to *_add endpoints
    
    Previously only release_add supported deduplication.
    This commit aligns other _add endpoints with it

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/1249/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/1249/console

Harbormaster failed remote builds in B20503: Diff 19459!Apr 7 2021, 4:18 PM
vlorentz added a comment.Apr 7 2021, 4:20 PM

Please don't change the signature to accommodate implementation details of the function

KShivendu updated this revision to Diff 19463.Apr 7 2021, 4:49 PM

Updating D5419: Cassandra: Deduplicate lists passed to *_add endpoints

swh-public-ci added a comment.Apr 7 2021, 4:55 PM

Build has FAILED

Patch application report for D5419 (id=19463)

Rebasing onto 39507b24d0...

First, rewinding head to replay your work on top of it...
Applying: Cassandra: Deduplicate lists passed to *_add endpoints
Changes applied before test
commit 9e1a825dbe11a214fe84b0a44d2b390bca85e712
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Mon Apr 5 17:09:33 2021 +0530

    Cassandra: Deduplicate lists passed to *_add endpoints
    
    Previously only release_add supported deduplication.
    This commit aligns other _add endpoints with it

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/1250/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/1250/console

Harbormaster failed remote builds in B20508: Diff 19463!Apr 7 2021, 4:55 PM
KShivendu added a comment.Apr 8 2021, 11:47 AM

These objects has an id attribute. You can use it for deduplication (eg. via a dict)

Not all objects had id so I used swhid. But some of the tests are failing.

Exactly these 3 tests are failing for each of in_memory, cassandra, and api_client :

  1. test_content_add_collision
  2. test_release_add
  3. test_release_add_twice

I have ideas of fix these but I think they are most likely wrong. So can you please review these and tell me what can be done ?

vlorentz added a comment.Apr 8 2021, 11:51 AM
In D5419#138253, @KShivendu wrote:

Not all objects had id so I used swhid. But some of the tests are failing.

Only content does not have an id, because they should only be deduplicated using all hashes, hence the test_content_add_collision failure

I have ideas of fix these but I think they are most likely wrong. So can you please review these and tell me what can be done ?

What do you think should be done for releases?

KShivendu updated this revision to Diff 19532.Apr 8 2021, 6:00 PM

Updating D5419: Cassandra: Fixed failing tests

swh-public-ci added a comment.Apr 8 2021, 6:07 PM

Build is green

Patch application report for D5419 (id=19532)

Rebasing onto ccaac113a0...

First, rewinding head to replay your work on top of it...
Applying: Cassandra: Deduplicate lists passed to *_add endpoints
Changes applied before test
commit 22b3a91e5e180d85483080de05e861cba98070ad
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Mon Apr 5 17:09:33 2021 +0530

    Cassandra: Deduplicate lists passed to *_add endpoints
    
    Previously only release_add supported deduplication.
    This commit aligns other _add endpoints with it

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/1251/ for more details.

Harbormaster completed remote builds in B20569: Diff 19532.Apr 8 2021, 6:07 PM
KShivendu planned changes to this revision.Apr 8 2021, 6:13 PM

What do you think should be done for releases?

Turns out I did some mistakes while renaming the variables.

KShivendu requested review of this revision.Apr 8 2021, 6:19 PM
KShivendu added inline comments.Apr 8 2021, 6:23 PM
swh/storage/cassandra/storage.py
204

Not covered. Should I do something about this?

vlorentz requested changes to this revision.Apr 8 2021, 6:25 PM
vlorentz added inline comments.
swh/storage/cassandra/storage.py
195–200

use a tuple

204

it's intentional, see test_cassandra.py

359

d.id (swhid() is formatted based on the id, so this just adds useless computation)

486–487

r.id

626–627

s.id

896–897

o.url

897–901

no need for set() now

This revision now requires changes to proceed.Apr 8 2021, 6:25 PM
KShivendu updated this revision to Diff 19547.Apr 9 2021, 10:54 AM

Updating D5419: Cassandra: Deduplicate lists passed to *_add endpoints

vlorentz added inline comments.Apr 9 2021, 11:09 AM
swh/storage/cassandra/storage.py
195–200

you don't need to convert to strings

897–901

The comment is outdated, and this line actually does nothing now (it sorts a list using the list's own order...)

swh-public-ci added a comment.Apr 9 2021, 11:14 AM

Build was aborted

Patch application report for D5419 (id=19547)

Rebasing onto ccaac113a0...

First, rewinding head to replay your work on top of it...
Applying: Cassandra: Deduplicate lists passed to *_add endpoints
Changes applied before test
commit 77989fe5f2800a39fa822806fe4181b1cb3362c4
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Mon Apr 5 17:09:33 2021 +0530

    Cassandra: Deduplicate lists passed to *_add endpoints
    
    Previously only release_add supported deduplication.
    This commit aligns other _add endpoints with it

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/1252/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/1252/console

Harbormaster failed remote builds in B20585: Diff 19547!Apr 9 2021, 11:14 AM
swh-public-ci added a comment.Apr 9 2021, 11:22 AM

Build is green

Patch application report for D5419 (id=19547)

Rebasing onto ccaac113a0...

First, rewinding head to replay your work on top of it...
Applying: Cassandra: Deduplicate lists passed to *_add endpoints
Changes applied before test
commit a24f61d1b3d35007f6773b5843a75ab0cfa0ad10
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Mon Apr 5 17:09:33 2021 +0530

    Cassandra: Deduplicate lists passed to *_add endpoints
    
    Previously only release_add supported deduplication.
    This commit aligns other _add endpoints with it

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/1253/ for more details.

Harbormaster completed remote builds in B20585: Diff 19547.Apr 9 2021, 11:22 AM
KShivendu updated this revision to Diff 19563.Apr 9 2021, 12:22 PM

Updating D5419: Cassandra: Deduplicate lists passed to *_add endpoints

swh-public-ci added a comment.Apr 9 2021, 12:27 PM

Build is green

Patch application report for D5419 (id=19563)

Rebasing onto ccaac113a0...

First, rewinding head to replay your work on top of it...
Applying: Cassandra: Deduplicate lists passed to *_add endpoints
Changes applied before test
commit 551c51d543a57c015655d7ababf373e1229bd420
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Mon Apr 5 17:09:33 2021 +0530

    Cassandra: Deduplicate lists passed to *_add endpoints
    
    Previously only release_add supported deduplication.
    This commit aligns other _add endpoints with it

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/1254/ for more details.

Harbormaster completed remote builds in B20596: Diff 19563.Apr 9 2021, 12:27 PM
vlorentz accepted this revision.Apr 12 2021, 1:10 PM
This revision is now accepted and ready to land.Apr 12 2021, 1:10 PM
KShivendu updated this revision to Diff 19605.Apr 12 2021, 1:29 PM

Updating D5419: Cassandra: Deduplicate lists passed to *_add endpoints

This revision was landed with ongoing or failed builds.Apr 12 2021, 1:30 PM
Closed by commit rDSTOc96942b40648: Cassandra: Deduplicate lists passed to *_add endpoints (authored by KShivendu). · Explain Why
This revision was automatically updated to reflect the committed changes.
KShivendu added a commit: rDSTOc96942b40648: Cassandra: Deduplicate lists passed to *_add endpoints.
swh-public-ci added a comment.Apr 12 2021, 1:35 PM

Build is green

Patch application report for D5419 (id=19605)

Rebasing onto 933289e2ef...

Current branch diff-target is up to date.
Changes applied before test
commit c96942b4064866cda0a36bbbc865462d0d58b6ca
Author: KShivendu <shivendu@iitbhilai.ac.in>
Date:   Mon Apr 5 17:09:33 2021 +0530

    Cassandra: Deduplicate lists passed to *_add endpoints
    
    Previously only release_add supported deduplication.
    This commit aligns other _add endpoints with it

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/1257/ for more details.

Harbormaster completed remote builds in B20637: Diff 19605.Apr 12 2021, 1:35 PM

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
1 line--
swh/
storage/
cassandra/
41 lines97%
Loading...

Diff 19605

View Options

CONTRIBUTORS

Loading...
View Options

swh/storage/cassandra/storage.py

Loading...
About · Developer information · Legal