Paths

Table of Contentst

-
swh/storage/
-
storage/
-
cassandra/
4/8
cql.py
4/8
storage.py
4/8
in_memory.py
-
tests/
4/8
test_cassandra.py

Differential D6228

content_get: Fetch rows concurrently
ClosedPublic
Actions

Authored by vlorentz on Sep 9 2021, 3:31 PM.

Details

Reviewers

olasd

Group Reviewers

Reviewers

Commits

rDSTO0570a426efa5: content_get: Fetch rows concurrently

Summary

Instead of fetching them one-by-one, with the very high latency this entails.

This is preliminary work to make directory_ls less painfully slow.

Diff Detail

Repository

rDSTO Storage manager

Branch

cassandra-perfs

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 23484
Build 36638: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 36637: arc lint + arc unit

Event Timeline

vlorentz created this revision.Sep 9 2021, 3:31 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptSep 9 2021, 3:31 PM

Build has FAILED

Patch application report for D6228 (id=22538)

Rebasing onto 50fb54f212...

Current branch diff-target is up to date.

Changes applied before test

commit 680d12149bb4e6f2c6c824c4f2684f80cadba38f
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Sep 9 15:30:09 2021 +0200

    content_get: Fetch rows concurrently
    
    Instead of fetching them one-by-one, with the very high latency this entails.
    
    This is preliminary work to make `directory_ls` less painfully slow.

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

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 9 2021, 3:38 PM

Harbormaster failed remote builds in B23483: Diff 22538!

update in-mem

vlorentz added a child revision: D6229: cassandra: Make directory_ls fetch contents in batch instead of one-by-one.Sep 9 2021, 3:45 PM

Build is green

Patch application report for D6228 (id=22539)

Rebasing onto 50fb54f212...

Current branch diff-target is up to date.

Changes applied before test

commit 0570a426efa5c89f85b407b5c3c53d6e9615cd06
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Sep 9 15:30:09 2021 +0200

    content_get: Fetch rows concurrently
    
    Instead of fetching them one-by-one, with the very high latency this entails.
    
    This is preliminary work to make `directory_ls` less painfully slow.

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

Harbormaster completed remote builds in B23484: Diff 22539.Sep 9 2021, 3:51 PM

vlorentz requested review of this revision.Sep 9 2021, 3:51 PM

Conditionally accepted on whether the inconsistency between _execute_with_retries and _execute_many_with_retries is worth it.

swh/storage/cassandra/cql.py
292–293	Shouldn't this be turned into a generator for consistency? Or the other method kept eager? Is there a point where we do `_execute_many_with_retries` but don't want to keep all results?
312	Use a list comprehension?
314–315	ugh generators.

This revision is now accepted and ready to land.Sep 10 2021, 4:48 PM

vlorentz added inline comments.Sep 10 2021, 4:58 PM

swh/storage/cassandra/cql.py
292–293	Shouldn't this be turned into a generator for consistency? Or the other method kept eager? ResultSet is lazy. https://docs.datastax.com/en/developer/python-driver/3.24/api/cassandra/cluster/#cassandra.cluster.ResultSet Is there a point where we do _execute_many_with_retries but don't want to keep all results? In `_add_many`.
314–315	ikr... as usual it took me some time to figure it out

Closed by commit rDSTO0570a426efa5: content_get: Fetch rows concurrently (authored by vlorentz). · Explain WhySep 10 2021, 6:09 PM

This revision was automatically updated to reflect the committed changes.

vlorentz added a commit: rDSTO0570a426efa5: content_get: Fetch rows concurrently.

vsellier mentioned this in T3573: [cassandra] directory and content read benchmarks.Sep 15 2021, 11:11 AM

Revision Contents
Changeset List

Path

Size

Coverage (All)

Coverage (Touched)

swh/

storage/

cassandra/

cql.py

20 lines

98%

storage.py

54 lines

97%

in_memory.py

7 lines

99%

tests/

test_cassandra.py

12 lines

91%

Diff 22539

View Options

swh/storage/cassandra/cql.py

Show First 20 Lines • Show All 59 Lines • ▼ Show 20 Lines	def get_package_info(self, url):
# currently only use the first one, but if the first one		# currently only use the first one, but if the first one
# fails, we should try the second one and so on.		# fails, we should try the second one and so on.
integrity = self._integrityByUrl[url]		integrity = self._integrityByUrl[url]
yield url, {"url": url, "raw": {"url": url, "integrity": integrity}}		yield url, {"url": url, "raw": {"url": url, "integrity": integrity}}

def resolve_revision_from(		def resolve_revision_from(
self, known_artifacts: Dict, artifact_metadata: Dict		self, known_artifacts: Dict, artifact_metadata: Dict
) -> Optional[bytes]:		) -> Optional[bytes]:

for rev_id, known_artifact in known_artifacts.items():		for rev_id, known_artifact in known_artifacts.items():
		try:
		lewoUnsubmitted Not Done Inline Actions We should catch a `keyError` exception instead since it could come from several keys. lewo: We should catch a `keyError` exception instead since it could come from several keys.
		ardumontAuthorUnsubmitted Done Inline Actions yes ardumont: yes
		douarddaUnsubmitted Not Done Inline Actions I agree a try/except KeyError is better here. Also why exit the for loop? Isn't there any chance the searched item is found later in the for loop? douardda: I agree a try/except KeyError is better here. Also why exit the for loop? Isn't there any…
		ardumontAuthorUnsubmitted Done Inline Actions Yes, i changed it locally already. Thanks. I can only assume i was thinking (partially wrongly), the other revisions must be in the same bad format. It was true in staging when i first hit that issue, that's possibly not the case in production. ardumont: Yes, i changed it locally already. Thanks. I can only assume i was thinking (partially…
known_integrity = known_artifact["extrinsic"]["raw"]["integrity"]		known_integrity = known_artifact["extrinsic"]["raw"]["integrity"]
		lewoUnsubmitted Done Inline Actions When the sources.json file is parsed, we check the integrity attribute is present. So, I think it should never be missing. In this case, I think it would improve code clearness to move this expression out of the try block. lewo: When the sources.json file is parsed, we check the integrity attribute is present. So, I think…
		ardumontAuthorUnsubmitted Done Inline Actions ack, adapted accordingly, thx. ardumont: ack, adapted accordingly, thx.
if artifact_metadata["integrity"] == known_integrity:		if artifact_metadata["integrity"] == known_integrity:
return rev_id		return rev_id
		vlorentzUnsubmitted Not Done Inline Actions I'd rather call it "unexpected" than "divergent" (divergent evokes an infinite data structure for me) vlorentz: I'd rather call it "unexpected" than "divergent" (divergent evokes an infinite data structure…
		except KeyError:
		# metadata field for the revision is not as expected by the loader
		# nixguix. We consider this not the right revision
		# continue checking the other revisions
		continue
return None		return None

def extra_branches(self) -> Dict[bytes, Mapping[str, Any]]:		def extra_branches(self) -> Dict[bytes, Mapping[str, Any]]:
		vlorentzUnsubmitted Not Done Inline Actions why not `log.exception`? it looks important enough to deserve it vlorentz: why not `log.exception`? it looks important enough to deserve it
"""We add a branch to the snapshot called 'evaluation' pointing to the		"""We add a branch to the snapshot called 'evaluation' pointing to the
revision used to generate the sources.json file. This revision		revision used to generate the sources.json file. This revision
is specified in the sources.json file itself. For the nixpkgs		is specified in the sources.json file itself. For the nixpkgs
origin, this revision is coming from the		origin, this revision is coming from the
github.com/nixos/nixpkgs repository.		github.com/nixos/nixpkgs repository.

Note this repository is not loaded explicitly. So, this		Note this repository is not loaded explicitly. So, this
pointer can target a nonexistent revision for a time. However,		pointer can target a nonexistent revision for a time. However,
▲ Show 20 Lines • Show All 101 Lines • Show Last 20 Lines

View Options

swh/storage/cassandra/storage.py

Show First 20 Lines • Show All 59 Lines • ▼ Show 20 Lines	def get_package_info(self, url):
# currently only use the first one, but if the first one		# currently only use the first one, but if the first one
# fails, we should try the second one and so on.		# fails, we should try the second one and so on.
integrity = self._integrityByUrl[url]		integrity = self._integrityByUrl[url]
yield url, {"url": url, "raw": {"url": url, "integrity": integrity}}		yield url, {"url": url, "raw": {"url": url, "integrity": integrity}}

def resolve_revision_from(		def resolve_revision_from(
self, known_artifacts: Dict, artifact_metadata: Dict		self, known_artifacts: Dict, artifact_metadata: Dict
) -> Optional[bytes]:		) -> Optional[bytes]:

for rev_id, known_artifact in known_artifacts.items():		for rev_id, known_artifact in known_artifacts.items():
		try:
		lewoUnsubmitted Not Done Inline Actions We should catch a `keyError` exception instead since it could come from several keys. lewo: We should catch a `keyError` exception instead since it could come from several keys.
		ardumontAuthorUnsubmitted Done Inline Actions yes ardumont: yes
		douarddaUnsubmitted Not Done Inline Actions I agree a try/except KeyError is better here. Also why exit the for loop? Isn't there any chance the searched item is found later in the for loop? douardda: I agree a try/except KeyError is better here. Also why exit the for loop? Isn't there any…
		ardumontAuthorUnsubmitted Done Inline Actions Yes, i changed it locally already. Thanks. I can only assume i was thinking (partially wrongly), the other revisions must be in the same bad format. It was true in staging when i first hit that issue, that's possibly not the case in production. ardumont: Yes, i changed it locally already. Thanks. I can only assume i was thinking (partially…
known_integrity = known_artifact["extrinsic"]["raw"]["integrity"]		known_integrity = known_artifact["extrinsic"]["raw"]["integrity"]
		lewoUnsubmitted Done Inline Actions When the sources.json file is parsed, we check the integrity attribute is present. So, I think it should never be missing. In this case, I think it would improve code clearness to move this expression out of the try block. lewo: When the sources.json file is parsed, we check the integrity attribute is present. So, I think…
		ardumontAuthorUnsubmitted Done Inline Actions ack, adapted accordingly, thx. ardumont: ack, adapted accordingly, thx.
if artifact_metadata["integrity"] == known_integrity:		if artifact_metadata["integrity"] == known_integrity:
return rev_id		return rev_id
		vlorentzUnsubmitted Not Done Inline Actions I'd rather call it "unexpected" than "divergent" (divergent evokes an infinite data structure for me) vlorentz: I'd rather call it "unexpected" than "divergent" (divergent evokes an infinite data structure…
		except KeyError:
		# metadata field for the revision is not as expected by the loader
		# nixguix. We consider this not the right revision
		# continue checking the other revisions
		continue
return None		return None

def extra_branches(self) -> Dict[bytes, Mapping[str, Any]]:		def extra_branches(self) -> Dict[bytes, Mapping[str, Any]]:
		vlorentzUnsubmitted Not Done Inline Actions why not `log.exception`? it looks important enough to deserve it vlorentz: why not `log.exception`? it looks important enough to deserve it
"""We add a branch to the snapshot called 'evaluation' pointing to the		"""We add a branch to the snapshot called 'evaluation' pointing to the
revision used to generate the sources.json file. This revision		revision used to generate the sources.json file. This revision
is specified in the sources.json file itself. For the nixpkgs		is specified in the sources.json file itself. For the nixpkgs
origin, this revision is coming from the		origin, this revision is coming from the
github.com/nixos/nixpkgs repository.		github.com/nixos/nixpkgs repository.

Note this repository is not loaded explicitly. So, this		Note this repository is not loaded explicitly. So, this
pointer can target a nonexistent revision for a time. However,		pointer can target a nonexistent revision for a time. However,
▲ Show 20 Lines • Show All 101 Lines • Show Last 20 Lines

View Options

swh/storage/in_memory.py

Show First 20 Lines • Show All 59 Lines • ▼ Show 20 Lines	def get_package_info(self, url):
# currently only use the first one, but if the first one		# currently only use the first one, but if the first one
# fails, we should try the second one and so on.		# fails, we should try the second one and so on.
integrity = self._integrityByUrl[url]		integrity = self._integrityByUrl[url]
yield url, {"url": url, "raw": {"url": url, "integrity": integrity}}		yield url, {"url": url, "raw": {"url": url, "integrity": integrity}}

def resolve_revision_from(		def resolve_revision_from(
self, known_artifacts: Dict, artifact_metadata: Dict		self, known_artifacts: Dict, artifact_metadata: Dict
) -> Optional[bytes]:		) -> Optional[bytes]:

for rev_id, known_artifact in known_artifacts.items():		for rev_id, known_artifact in known_artifacts.items():
		try:
		lewoUnsubmitted Not Done Inline Actions We should catch a `keyError` exception instead since it could come from several keys. lewo: We should catch a `keyError` exception instead since it could come from several keys.
		ardumontAuthorUnsubmitted Done Inline Actions yes ardumont: yes
		douarddaUnsubmitted Not Done Inline Actions I agree a try/except KeyError is better here. Also why exit the for loop? Isn't there any chance the searched item is found later in the for loop? douardda: I agree a try/except KeyError is better here. Also why exit the for loop? Isn't there any…
		ardumontAuthorUnsubmitted Done Inline Actions Yes, i changed it locally already. Thanks. I can only assume i was thinking (partially wrongly), the other revisions must be in the same bad format. It was true in staging when i first hit that issue, that's possibly not the case in production. ardumont: Yes, i changed it locally already. Thanks. I can only assume i was thinking (partially…
known_integrity = known_artifact["extrinsic"]["raw"]["integrity"]		known_integrity = known_artifact["extrinsic"]["raw"]["integrity"]
		lewoUnsubmitted Done Inline Actions When the sources.json file is parsed, we check the integrity attribute is present. So, I think it should never be missing. In this case, I think it would improve code clearness to move this expression out of the try block. lewo: When the sources.json file is parsed, we check the integrity attribute is present. So, I think…
		ardumontAuthorUnsubmitted Done Inline Actions ack, adapted accordingly, thx. ardumont: ack, adapted accordingly, thx.
if artifact_metadata["integrity"] == known_integrity:		if artifact_metadata["integrity"] == known_integrity:
return rev_id		return rev_id
		vlorentzUnsubmitted Not Done Inline Actions I'd rather call it "unexpected" than "divergent" (divergent evokes an infinite data structure for me) vlorentz: I'd rather call it "unexpected" than "divergent" (divergent evokes an infinite data structure…
		except KeyError:
		# metadata field for the revision is not as expected by the loader
		# nixguix. We consider this not the right revision
		# continue checking the other revisions
		continue
return None		return None

def extra_branches(self) -> Dict[bytes, Mapping[str, Any]]:		def extra_branches(self) -> Dict[bytes, Mapping[str, Any]]:
		vlorentzUnsubmitted Not Done Inline Actions why not `log.exception`? it looks important enough to deserve it vlorentz: why not `log.exception`? it looks important enough to deserve it
"""We add a branch to the snapshot called 'evaluation' pointing to the		"""We add a branch to the snapshot called 'evaluation' pointing to the
revision used to generate the sources.json file. This revision		revision used to generate the sources.json file. This revision
is specified in the sources.json file itself. For the nixpkgs		is specified in the sources.json file itself. For the nixpkgs
origin, this revision is coming from the		origin, this revision is coming from the
github.com/nixos/nixpkgs repository.		github.com/nixos/nixpkgs repository.

Note this repository is not loaded explicitly. So, this		Note this repository is not loaded explicitly. So, this
pointer can target a nonexistent revision for a time. However,		pointer can target a nonexistent revision for a time. However,
▲ Show 20 Lines • Show All 101 Lines • Show Last 20 Lines

View Options

swh/storage/tests/test_cassandra.py

Show First 20 Lines • Show All 59 Lines • ▼ Show 20 Lines	def get_package_info(self, url):
# currently only use the first one, but if the first one		# currently only use the first one, but if the first one
# fails, we should try the second one and so on.		# fails, we should try the second one and so on.
integrity = self._integrityByUrl[url]		integrity = self._integrityByUrl[url]
yield url, {"url": url, "raw": {"url": url, "integrity": integrity}}		yield url, {"url": url, "raw": {"url": url, "integrity": integrity}}

def resolve_revision_from(		def resolve_revision_from(
self, known_artifacts: Dict, artifact_metadata: Dict		self, known_artifacts: Dict, artifact_metadata: Dict
) -> Optional[bytes]:		) -> Optional[bytes]:

for rev_id, known_artifact in known_artifacts.items():		for rev_id, known_artifact in known_artifacts.items():
		try:
		lewoUnsubmitted Not Done Inline Actions We should catch a `keyError` exception instead since it could come from several keys. lewo: We should catch a `keyError` exception instead since it could come from several keys.
		ardumontAuthorUnsubmitted Done Inline Actions yes ardumont: yes
		douarddaUnsubmitted Not Done Inline Actions I agree a try/except KeyError is better here. Also why exit the for loop? Isn't there any chance the searched item is found later in the for loop? douardda: I agree a try/except KeyError is better here. Also why exit the for loop? Isn't there any…
		ardumontAuthorUnsubmitted Done Inline Actions Yes, i changed it locally already. Thanks. I can only assume i was thinking (partially wrongly), the other revisions must be in the same bad format. It was true in staging when i first hit that issue, that's possibly not the case in production. ardumont: Yes, i changed it locally already. Thanks. I can only assume i was thinking (partially…
known_integrity = known_artifact["extrinsic"]["raw"]["integrity"]		known_integrity = known_artifact["extrinsic"]["raw"]["integrity"]
		lewoUnsubmitted Done Inline Actions When the sources.json file is parsed, we check the integrity attribute is present. So, I think it should never be missing. In this case, I think it would improve code clearness to move this expression out of the try block. lewo: When the sources.json file is parsed, we check the integrity attribute is present. So, I think…
		ardumontAuthorUnsubmitted Done Inline Actions ack, adapted accordingly, thx. ardumont: ack, adapted accordingly, thx.
if artifact_metadata["integrity"] == known_integrity:		if artifact_metadata["integrity"] == known_integrity:
return rev_id		return rev_id
		vlorentzUnsubmitted Not Done Inline Actions I'd rather call it "unexpected" than "divergent" (divergent evokes an infinite data structure for me) vlorentz: I'd rather call it "unexpected" than "divergent" (divergent evokes an infinite data structure…
		except KeyError:
		# metadata field for the revision is not as expected by the loader
		# nixguix. We consider this not the right revision
		# continue checking the other revisions
		continue
return None		return None

def extra_branches(self) -> Dict[bytes, Mapping[str, Any]]:		def extra_branches(self) -> Dict[bytes, Mapping[str, Any]]:
		vlorentzUnsubmitted Not Done Inline Actions why not `log.exception`? it looks important enough to deserve it vlorentz: why not `log.exception`? it looks important enough to deserve it
"""We add a branch to the snapshot called 'evaluation' pointing to the		"""We add a branch to the snapshot called 'evaluation' pointing to the
revision used to generate the sources.json file. This revision		revision used to generate the sources.json file. This revision
is specified in the sources.json file itself. For the nixpkgs		is specified in the sources.json file itself. For the nixpkgs
origin, this revision is coming from the		origin, this revision is coming from the
github.com/nixos/nixpkgs repository.		github.com/nixos/nixpkgs repository.

Note this repository is not loaded explicitly. So, this		Note this repository is not loaded explicitly. So, this
pointer can target a nonexistent revision for a time. However,		pointer can target a nonexistent revision for a time. However,
▲ Show 20 Lines • Show All 101 Lines • Show Last 20 Lines

content_get: Fetch rows concurrentlyClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D6228 (id=22538)

Changes applied before test

Patch application report for D6228 (id=22539)

Changes applied before test

Revision ContentsChangeset List

Diff 22539

swh/storage/cassandra/cql.py

swh/storage/cassandra/storage.py

swh/storage/in_memory.py

swh/storage/tests/test_cassandra.py

content_get: Fetch rows concurrently
ClosedPublic
Actions

Revision Contents
Changeset List