Allow prefix to be specified per source (#2650) #2827

amarjandu · 2021-02-23T18:39:35Z

PR title references issue
Title of main commit references issue
PR is linked to Zenhub issue

Author (reindex)

Added r tag to commit title _{or this PR does not require reindexing}
Added reindex label to PR _{or this PR does not require reindexing}

Author (freebies & chains)

Freebies are blocked on this PR _{or there are no freebies in this PR}
Freebies are referenced in commit titles _{or there are no freebies in this PR}
This PR is blocked by previous PR in the chain _{or this PR is not chained to another PR}
Added chain label to the blocking PR _{or this PR is not chained to another PR}

Author (upgrading)

Documented upgrading of deployments in UPGRADING.rst _{or this PR does not require upgrading}
Added u tag to commit title _{or this PR does not require upgrading}
Added upgrade label to PR _{or this PR does not require upgrading}

Author (requirements, before every review)

Ran make requirements_update _{or this PR leaves requirements*.txt, common.mk and Makefile untouched}
Added R tag to commit title _{or this PR leaves requirements*.txt untouched}
Added reqs label to PR _{or this PR leaves requirements*.txt untouched}

Author (before every review)

make integration_test passes in personal deployment _{or this PR does not touch functionality that could break the IT}
Rebased branch on develop, squashed old fixups

Primary reviewer (after approval)

Commented on demo expectations _{or labelled PR as no demo}
Decided if PR can be labeled no sandbox
PR title is appropriate as title of merge commit
Moved ticket to Approved column
Assigned PR to an operator

Operator (before pushing merge the commit)

Operator (after pushing the merge commit)

Moved freebies to Merged column _{or there are no freebies in this PR}
Shortened the PR chain _{or this PR is not the base of another PR}
Verified that N reviews labelling is accurate
Pushed merge commit to Gitlab _{or this changes can be pushed later, together with another PR}
Deleted PR branch from Github and Gitlab

Operator (reindex)

Purchased BQ slot committment in dev _{or this PR does not require reindexing dev}
Started reindex in dev _{or this PR does not require reindexing dev}
Deleted BQ slot committment in dev _{or this PR does not require reindexing dev}
Checked for failures in dev _{or this PR does not require reindexing dev}
Purchased BQ slot committment in prod _{or this PR does not require reindexing prod}
Started reindex in prod _{or this PR does not require reindexing prod}
Deleted BQ slot committment in prod _{or this PR does not require reindexing prod}
Checked for failures in prod _{or this PR does not require reindexing prod}

Operator

Unassigned PR

amarjandu · 2021-02-23T18:40:50Z

Testing will fail; env vars need to be updated if we decide to go with this change.
Waiting on #2650 (comment)

amarjandu · 2021-02-23T18:48:35Z

Need to update the scripts/reindex.py and inspect its usage in pipelines.

hannes-ucsc

Unfortunately, the spec in the ticket has been obsoleted by recent changes and I failed to keep it up to date. The prefix should be specific to the source, not the catalog. I'll update the ticket.

hannes-ucsc · 2021-02-24T18:16:55Z

UPGRADING.rst

+
+The syntax of the value of the AZUL_CATALOGS environment variable was modified
+to include a prefix value. To upgrade a deployment, append every catalog entry
+in that variable with a prefix ``:foo``.


Prefixes have to be hexadecimal so this isn't a representative example.

hannes-ucsc · 2021-02-24T18:31:34Z

src/azul/__init__.py

+        for catalog_name, catalog in catalogs.items():
+            reject('/' in catalog_name,
+                   'It appears AZUL_CATALOGS was not upgraded to include atlas names.')
+            reject(catalog.prefix == '')


Empty prefixes should be accepted. In fact, they are the norm.

hannes-ucsc · 2021-02-24T21:47:06Z

@amarjandu note my edit to the description of the underlying ticket.

codecov · 2021-03-09T01:10:46Z

Codecov Report

Merging #2827 (2a478bf) into develop (3c6079d) will increase coverage by 0.01%.
The diff coverage is 82.60%.

❗ Current head 2a478bf differs from pull request most recent head 9c23cb7. Consider uploading reports for the commit 9c23cb7 to get more accurate results

@@             Coverage Diff             @@
##           develop    #2827      +/-   ##
===========================================
+ Coverage    82.59%   82.60%   +0.01%     
===========================================
  Files          117      117              
  Lines        12221    12229       +8     
===========================================
+ Hits         10094    10102       +8     
  Misses        2127     2127

Impacted Files	Coverage Δ
src/azul/azulclient.py	`37.98% <0.00%> (-0.14%)`	⬇️
src/azul/plugins/repository/tdr/__init__.py	`82.72% <ø> (ø)`
src/azul/plugins/repository/dss/__init__.py	`80.15% <40.00%> (-1.10%)`	⬇️
src/azul/__init__.py	`82.08% <100.00%> (+0.15%)`	⬆️
src/azul/indexer/__init__.py	`98.61% <100.00%> (+0.12%)`	⬆️
src/azul/indexer/document.py	`95.45% <100.00%> (ø)`
src/azul/terra.py	`50.29% <100.00%> (+0.60%)`	⬆️
test/indexer/test_indexer_controller.py	`100.00% <100.00%> (ø)`
test/service/test_repository_proxy.py	`99.37% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 377112d...9c23cb7. Read the comment docs.

coveralls · 2021-03-09T01:11:13Z

Coverage increased (+0.01%) to 83.129% when pulling 9c23cb7 on issues/amar/2650-catalog-prefix into 377112d on develop.

hannes-ucsc · 2021-03-09T20:56:55Z

UPGRADING.rst

+#2650 Add prefix to sources
+===========================
+
+The syntax for the value of the ``AZUL_TDR_SOURCES`` and ``AZUL_TDR_…_SOURCES``


Suggested change

The syntax for the value of the ``AZUL_TDR_SOURCES`` and ``AZUL_TDR_…_SOURCES``

The syntax of ``AZUL_TDR_SOURCES`` and ``AZUL_TDR_…_SOURCES``

hannes-ucsc · 2021-03-09T20:57:12Z

UPGRADING.rst

+===========================
+
+The syntax for the value of the ``AZUL_TDR_SOURCES`` and ``AZUL_TDR_…_SOURCES``
+environment variables were modified to include a prefix. To upgrade a


Suggested change

environment variables were modified to include a prefix. To upgrade a

environment variables was modified to include a UUID prefix. To upgrade a

deployments/sandbox/environment.py

hannes-ucsc · 2021-03-09T20:59:07Z

scripts/reindex.py

-                         'default) no partitioning occurs, the DSS is queried locally and the indexer notification '
-                         'endpoint is invoked for each bundle individually and concurrently using worker threads. '
-                         'This is magnitudes slower that partitioned indexing.')
+                         'The lambda queries the data repository and queues a notification for each matching bundle. '


Suggested change

'The lambda queries the data repository and queues a notification for each matching bundle. '

'The lambda queries the repository and queues a notification for each matching bundle. '

hannes-ucsc · 2021-03-09T20:59:23Z

scripts/reindex.py

-                         'endpoint is invoked for each bundle individually and concurrently using worker threads. '
-                         'This is magnitudes slower that partitioned indexing.')
+                         'The lambda queries the data repository and queues a notification for each matching bundle. '
+                         'If 0 (the default) no partitioning occurs, the data repository is queried locally and the '


Suggested change

'If 0 (the default) no partitioning occurs, the data repository is queried locally and the '

'If 0 (the default) no partitioning occurs, the repository is queried locally and the '

hannes-ucsc · 2021-03-09T21:01:01Z

src/azul/azulclient.py

@@ -262,8 +259,8 @@ def remote_reindex_partition(self, message: JSON) -> None:
        bundle_fqids = self.list_bundles(catalog, source, prefix)
        bundle_fqids = self.filter_obsolete_bundle_versions(bundle_fqids)
        logger.info('After filtering obsolete versions, '
-                    '%i bundles remain in prefix %r of catalog %r',
-                    len(bundle_fqids), prefix, catalog)
+                    '%i bundles remain in prefix %r for the source %r in catalog %r',


Suggested change

'%i bundles remain in prefix %r for the source %r in catalog %r',

'%i bundles remain in prefix %r of source %r in catalog %r',

hannes-ucsc · 2021-03-09T21:01:39Z

src/azul/azulclient.py

+        logger.info('Successfully queued %i notification(s) for prefix %s for '
+                    'the source %r', num_messages, prefix, source)


Suggested change

logger.info('Successfully queued %i notification(s) for prefix %s for '

'the source %r', num_messages, prefix, source)

logger.info('Successfully queued %i notification(s) for prefix %s of '

'source %r', num_messages, prefix, source)

hannes-ucsc · 2021-03-09T21:03:48Z

src/azul/terra.py

@@ -75,6 +78,7 @@ class TDRSourceName(SourceName):
    project: str
    name: str
    is_snapshot: bool
+    prefix: str


Suggested change

prefix: str

prefix: str = ''

hannes-ucsc · 2021-03-09T21:04:10Z

test/indexer/test_tdr.py

+                                             is_snapshot=True,
+                                             prefix=''))


Suggested change

is_snapshot=True,

prefix=''))

is_snapshot=True))

hannes-ucsc · 2021-03-09T21:31:50Z

scripts/reindex.py

            else:
-                azul.reindex(catalog, args.prefix)
+                azul.reindex(catalog, '')


This is the only call site so it doesn't make sense to fix this here but retain the argument. Why did you remove the args.reindex option?

I think you are still reviewing this, but did you mean args.prefix?

Uhm, yes. Good catch.

I removed the args.prefixoption because it seemed like it would not be needed if we hardcode the prefix to the source. By allowing a prefix to be provided to azulclient.reindex the reindexed bundles would be those under {source.name.prefix} + {args.prefix}. At the time I was thinking that if we are required to redeploy to update the the source prefix, why not just make this the only way to specify the prefix.

Maybe it's better to retain the args.prefix, make the default value of it '', and update the description to mention that its used to narrow down the source prefix, rather than to reindex an entirely different prefix.

This is the only call site so it doesn't make sense to fix this here but retain the argument.

I would think that we want the azulclient to be robust enough to handle a prefix during reindex, so I opted to retain the prefix argument in that method vs removing it, even though its only used in this one call site.

I think there is a problem that we overload the term prefix in the repository plugins to mean source.name.prefix or partition prefix. By having distinct optional arguments for prefix and partition prefix within the methods of the repository plugin we can make make the client more flexible, and is probably something worth looking into in the long run.

amarjandu · 2021-03-19T21:30:11Z

pulling from review to rebase

hannes-ucsc · 2021-04-29T01:02:36Z

deployments/sandbox/environment.py

            ])
-            for catalog in ('dcp2ebi', 'it2ebi')
+            for catalog, prefix in (('dcp2ebi', '42'), ('it2ebi', ''))


The IT shortens the prefix until it finds enough bundles. If a configured prefix prevents the IT from finding any bundles, even with an IT prefix of '' then the configured prefix is wrong and actually includes no bundles. So if a prefix doesn't work for a source of an IT catalog, that prefix wouldn't work for that source on a non-IT catalog either since the source wouldn't contribute any bundles to said catalog.

amarjandu · 2021-04-29T18:50:05Z

add fixme:
"add tooling to aid in prefix choice"
create issue for this as well.

amarjandu · 2021-04-29T21:34:54Z

IT failed during the 10min wait period for queue stabilization. All notifications were handled by the indexer lambdas, restarted IT to double check.

hannes-ucsc · 2021-04-29T23:36:47Z

IT failed during the 10min wait period for queue stabilization. All notifications were handled by the indexer lambdas, restarted IT to double check.

I didn't see this until now, after my approval. Looks like you asked for approval first, then ran IT. That would be out of order.

If the IT passes please assign to @achave11, move to Approved and check the two checklist items, please.

Otherwise, back to square one (apply fix fixes, ask for re-review).

amarjandu · 2021-04-29T23:39:30Z

IT failed during the 10min wait period for queue stabilization. All notifications were handled by the indexer lambdas, restarted IT to double check.

I didn't see this until now, after my approval. Looks like you asked for approval first, then ran IT. That would be out of order.

If the IT passes please assign to @achave11, move to Approved and check the two checklist items, please.

Otherwise, back to square one (apply fix fixes, ask for re-review).

Sorry that was for my reference, I'll keep those types of notes off the PR thread to reduce clutter going forward, the IT does pass on my local deployment.

hannes-ucsc · 2021-04-30T01:08:20Z

Just prefix them with "Note to self" or similar.

theathorn · 2021-05-10T17:52:24Z

@hannes-ucsc "for demo, show evidence of diverse prefixes between catalogs in sandbox IT job logs on GitLab".

github-actions bot added the orange [process] Done by the Azul team label Feb 23, 2021

amarjandu force-pushed the issues/amar/2650-catalog-prefix branch from 88beae3 to 3e08211 Compare February 23, 2021 20:08

hannes-ucsc reviewed Feb 24, 2021

View reviewed changes

hannes-ucsc changed the title ~~Allow prefix to be specified per catalog (#2650)~~ Allow prefix to be specified per source (#2650) Feb 24, 2021

amarjandu force-pushed the issues/amar/2650-catalog-prefix branch 3 times, most recently from bd3ccd5 to 81a55ab Compare March 8, 2021 23:15

amarjandu force-pushed the issues/amar/2650-catalog-prefix branch from a89cbfb to 35533b3 Compare March 9, 2021 02:34

amarjandu added the upgrade [process] PR includes commit requiring manual upgrade label Mar 9, 2021

amarjandu assigned nadove-ucsc and hannes-ucsc and unassigned nadove-ucsc Mar 9, 2021

hannes-ucsc requested changes Mar 9, 2021

View reviewed changes

hannes-ucsc removed their assignment Mar 10, 2021

hannes-ucsc added the 1 review [process] Lead requested changes once label Mar 10, 2021

amarjandu force-pushed the issues/amar/2650-catalog-prefix branch 4 times, most recently from a5cfec5 to 8a80914 Compare March 15, 2021 18:18

amarjandu assigned hannes-ucsc Mar 15, 2021

hannes-ucsc force-pushed the develop branch from 6e5865b to 45d4ba2 Compare March 15, 2021 20:59

amarjandu force-pushed the issues/amar/2650-catalog-prefix branch from 8a80914 to c879124 Compare March 19, 2021 21:31

amarjandu unassigned hannes-ucsc Mar 22, 2021

melainalegaspi assigned hannes-ucsc Mar 29, 2021

hannes-ucsc removed their assignment Apr 26, 2021

amarjandu force-pushed the issues/amar/2650-catalog-prefix branch 2 times, most recently from 6a3e5b2 to e01184a Compare April 27, 2021 22:32

amarjandu assigned hannes-ucsc Apr 27, 2021

hannes-ucsc added the urgent label Apr 29, 2021

hannes-ucsc requested changes Apr 29, 2021

View reviewed changes

hannes-ucsc removed their assignment Apr 29, 2021

amarjandu assigned hannes-ucsc Apr 29, 2021

hannes-ucsc force-pushed the issues/amar/2650-catalog-prefix branch from 2a478bf to c8974d1 Compare April 29, 2021 23:27

hannes-ucsc approved these changes Apr 29, 2021

View reviewed changes

hannes-ucsc added demo ext [process] To be demonstrated to stakeholders no demo [process] Not to be demonstrated at the end of the sprint labels Apr 29, 2021

hannes-ucsc removed their assignment Apr 29, 2021

amarjandu assigned achave11-ucsc Apr 29, 2021

amarjandu and others added 3 commits April 30, 2021 08:21

Add FIXME to rename SourceName (#2843)

4e5d4c3

[u] Add prefix to end of TDR sources (#2650)

1ed5e74

Add FIXME for prefix tooling (#3027)

9c23cb7

achave11-ucsc force-pushed the issues/amar/2650-catalog-prefix branch from c8974d1 to 9c23cb7 Compare April 30, 2021 15:24

achave11-ucsc added the sandbox [process] Resolution is being verified in sandbox deployment label Apr 30, 2021

achave11-ucsc merged commit 20061e8 into develop May 3, 2021

achave11-ucsc deleted the issues/amar/2650-catalog-prefix branch May 3, 2021 23:35

achave11-ucsc removed their assignment May 4, 2021

theathorn removed the demo ext [process] To be demonstrated to stakeholders label May 10, 2021

theathorn removed the no demo [process] Not to be demonstrated at the end of the sprint label May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow prefix to be specified per source (#2650) #2827

Allow prefix to be specified per source (#2650) #2827

amarjandu commented Feb 23, 2021 •

edited by achave11-ucsc

Loading

amarjandu commented Feb 23, 2021

amarjandu commented Feb 23, 2021

hannes-ucsc left a comment

hannes-ucsc Feb 24, 2021

hannes-ucsc Feb 24, 2021

hannes-ucsc commented Feb 24, 2021

codecov bot commented Mar 9, 2021 •

edited

Loading

coveralls commented Mar 9, 2021 •

edited

Loading

hannes-ucsc Mar 9, 2021

hannes-ucsc Mar 9, 2021

hannes-ucsc Mar 9, 2021

hannes-ucsc Mar 9, 2021

hannes-ucsc Mar 9, 2021

hannes-ucsc Mar 9, 2021

hannes-ucsc Mar 9, 2021

hannes-ucsc Mar 9, 2021

hannes-ucsc Mar 9, 2021

amarjandu Mar 10, 2021

hannes-ucsc Mar 10, 2021

amarjandu Mar 11, 2021

amarjandu commented Mar 19, 2021

hannes-ucsc Apr 29, 2021

amarjandu commented Apr 29, 2021

amarjandu commented Apr 29, 2021

hannes-ucsc commented Apr 29, 2021

amarjandu commented Apr 29, 2021

hannes-ucsc commented Apr 30, 2021

theathorn commented May 10, 2021

	The syntax for the value of the ``AZUL_TDR_SOURCES`` and ``AZUL_TDR_…_SOURCES``
	The syntax of ``AZUL_TDR_SOURCES`` and ``AZUL_TDR_…_SOURCES``

	environment variables were modified to include a prefix. To upgrade a
	environment variables was modified to include a UUID prefix. To upgrade a

	'The lambda queries the data repository and queues a notification for each matching bundle. '
	'The lambda queries the repository and queues a notification for each matching bundle. '

	'If 0 (the default) no partitioning occurs, the data repository is queried locally and the '
	'If 0 (the default) no partitioning occurs, the repository is queried locally and the '

	'%i bundles remain in prefix %r for the source %r in catalog %r',
	'%i bundles remain in prefix %r of source %r in catalog %r',

		logger.info('Successfully queued %i notification(s) for prefix %s for '
		'the source %r', num_messages, prefix, source)

Allow prefix to be specified per source (#2650) #2827

Allow prefix to be specified per source (#2650) #2827

Conversation

amarjandu commented Feb 23, 2021 • edited by achave11-ucsc Loading

amarjandu commented Feb 23, 2021

amarjandu commented Feb 23, 2021

hannes-ucsc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannes-ucsc commented Feb 24, 2021

codecov bot commented Mar 9, 2021 • edited Loading

Codecov Report

coveralls commented Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amarjandu commented Mar 19, 2021

Choose a reason for hiding this comment

amarjandu commented Apr 29, 2021

amarjandu commented Apr 29, 2021

hannes-ucsc commented Apr 29, 2021

amarjandu commented Apr 29, 2021

hannes-ucsc commented Apr 30, 2021

theathorn commented May 10, 2021

amarjandu commented Feb 23, 2021 •

edited by achave11-ucsc

Loading

codecov bot commented Mar 9, 2021 •

edited

Loading

coveralls commented Mar 9, 2021 •

edited

Loading