fixup! [A] Workaround: Manifest content hash computation times out (#6123)

Author: dsotirho-ucsc

scripts/reindex.py

@@ -45,21 +45,15 @@
 parser.add_argument('--local',
                     default=False,
                     action='store_true',
-                    help=(
-                             '' if config.enable_bundle_notifications else '**DISABLED** '
-                         ) + (
-                             'Do not offload the listing of subgraphs to the indexer Lambda function. When '
-                             'this option is used, this script queries the repository without partitioning, '
-                             'and the indexer notification endpoint is invoked for each subgraph '
-                             'individually and concurrently using worker threads. This is magnitudes slower '
-                             'than remote (i.e. partitioned) indexing. If this option is not used (the '
-                             'default), the set of subgraphs matching the query is partitioned using the '
-                             'partition prefix length configured for each of the catalog sources being '
-                             'reindexed. Each query partition is processed independently and remotely by '
-                             'the indexer lambda. The index Lambda function queries the repository for each '
-                             'partition and queues a notification for each matching subgraph in the '
-                             'partition.'
-                         )
+                    help='Do not offload the listing of subgraphs to the indexer Lambda function. When this option is '
+                         'used, this script queries the repository without partitioning, and the indexer notification '
+                         'endpoint is invoked for each subgraph individually and concurrently using worker threads. '
+                         'This is magnitudes slower than remote (i.e. partitioned) indexing. If this option is not '
+                         'used (the default), the set of subgraphs matching the query is partitioned using the '
+                         'partition prefix length configured for each of the catalog sources being reindexed. Each '
+                         'query partition is processed independently and remotely by the indexer lambda. The index '
+                         'Lambda function queries the repository for each partition and queues a notification for each '
+                         'matching subgraph in the partition.')
                     )
 parser.add_argument('--catalogs',
                     nargs='+',
@@ -140,10 +134,6 @@ def main(argv: list[str]):
     deindex = args.deindex or (args.delete and not every_source)
     delete = args.delete and every_source
 
-    if args.local and not config.enable_bundle_notifications:
-        parser.error('Local reindexing is not available while bundle '
-                     'notifications are disabled.')
-
     if every_source:
         if deindex:
             parser.error('--deindex is incompatible with source `*`. Use --delete instead.')

src/azul/service/manifest_service.py

@@ -985,7 +985,9 @@ def manifest_key(self) -> ManifestKey:
         filter_string = repr(sort_frozen(freeze(self.filters.explicit)))
         # If incremental index changes are disabled, we don't need to worry
         # about individual bundles, only sources.
-        content_hash = str(self.manifest_hash(config.enable_bundle_notifications))
+        content_hash = str(
+            self.manifest_hash(by_bundle=config.enable_bundle_notifications)
+        )
         catalog = self.catalog
         format = self.format()
         manifest_hash_input = [
@@ -1150,29 +1152,38 @@ def _azul_file_url(self,
                                           **args))
 
     @cache
-    def manifest_hash(self, by_bundle: bool) -> int:
+    def manifest_hash(self, *, by_bundle: bool) -> int:
         """
-        Return a content hash for the manifest.
-
-        If `by_bundle` is True, the hash is computed from the fully-qualified
-        identifiers of all bundles containing files that match the current
-        filter. The return value approximates a hash of the content of the
-        manifest because a change of the file data requires a change to the file
-        metadata which requires a new bundle or bundle version.
-
-        If `by_bundle` is False, the hash is computed from the identifiers of
-        the sources from which projects/datasets containing files matching the
-        current filters were indexed. It's worth noting that a filter may match
-        a project/dataset but none of the project's files. For example, if a
-        project contains only files derived from either mouse brains or lion
-        hearts, the project will match the filter `species=lion and
-        organ=brain`, but none of its files will. If such a project/dataset is
-        added/removed to/from the index, the manifest hash returned for a given
-        filter will be different even though the contents of the manifest hasn't
-        changed, as no matching files were added or removed.
-
-        So while the hash computed from the sources is less sensitive than the
-        one computed from the bundles, it can be computed much more quickly.
+        Return a hash of the input this generator builds the manifest from. The
+        input is the set of ES documents from the files index. For two generator
+        instances g1 and g2 created at two different points in time, and any
+        boolean value b, if
+
+        g1.manifest_hash(by_bundle=b) == g2.manifest_hash(by_bundle=b)
+
+        then there is a high probability that the manifests generated by g1 and
+        g2 contain the same set of entries. This test can be used in deciding
+        whether g2 can reuse g1's manifest, thereby avoiding an expensive
+        operation. A false positive occurs when the hashes are equal but the
+        inputs differ. A false negative occurs when the hashes differ, but the
+        inputs are equal. False negatives are less problematic because they only
+        lead to redundant computations: the manifest is regenerated when it
+        could have been reused. False positives are problematic because they
+        lead to a manifest being reused erroneously, yielding an incorrect
+        manifest that is inconsistent with the input.
+
+        If ``by_bundle`` is True, the hash is computed from the fully-qualified
+        identifiers (FQID) of all bundles (subgraphs) containing files that
+        match the current filter. The rate of false negatives is low because a
+        change to any file entity requires a new bundle or a new bundle version,
+        both of which have different FQIDs, leading to a different hash. This
+        mode is slower and should be used if the index is changing or is likely
+        to change due to the incremental incorporation of bundles.
+
+        If ``by_bundle`` is False, the hash is instead computed from the set of
+        identifiers of the sources that contributed files matching the current
+        filters. This mode should *not* be used if the index is changing or is
+        likely to change due to the incremental incorporation of bundles.
         """
         log.debug('Computing content hash for manifest from %s using %r ...',
                   'bundles' if by_bundle else 'sources', self.filters)

test/service/test_manifest.py

@@ -126,6 +126,9 @@
     MutableJSON,
     MutableJSONs,
 )
+from azul_test_case import (
+    patch_config,
+)
 from indexer import (
     AnvilCannedBundleTestCase,
     CannedFileTestCase,
@@ -902,15 +905,11 @@ def test_manifest_filter_validation(self):
         response = requests.put(str(url))
         self.assertEqual(400, response.status_code, response.content)
 
-    @patch.object(type(config),
-                  'enable_bundle_notifications',
-                  new=PropertyMock(return_value=True))
+    @patch_config('enable_bundle_notifications', True)
     def test_content_disposition_header_with_notifications_enabled(self) -> None:
         self._test_content_disposition_header()
 
-    @patch.object(type(config),
-                  'enable_bundle_notifications',
-                  new=PropertyMock(return_value=False))
+    @patch_config('enable_bundle_notifications', False)
     def test_content_disposition_header_with_notifications_disabled(self) -> None:
         self._test_content_disposition_header()
 
@@ -1077,15 +1076,11 @@ def test_compact_metadata_cache(self, _time_until_object_expires: MagicMock):
                 self.assertEqual([2, 2], list(map(len, file_names.values())))
                 self.assertEqual([1, 1], list(map(len, map(set, file_names.values()))))
 
-    @patch.object(type(config),
-                  'enable_bundle_notifications',
-                  new=PropertyMock(return_value=True))
+    @patch_config('enable_bundle_notifications', True)
     def test_hash_validity_with_notifications_enabled(self) -> None:
         self._test_hash_validity()
 
-    @patch.object(type(config),
-                  'enable_bundle_notifications',
-                  new=PropertyMock(return_value=False))
+    @patch_config('enable_bundle_notifications', False)
     def test_hash_validity_with_notifications_disabled(self) -> None:
         self._test_hash_validity()