CUMULUS-3847: Remove any remaining ES indexing functions and tests #3897

charleshuang80 · 2025-01-08T19:04:17Z

…loud Metrics usage

Summary: Summary of changes

Addresses CUMULUS-3847: Remove any remaining ES indexing functions and tests

Changes

Detailed list or prose of changes
...

PR Checklist

Update CHANGELOG
Unit tests
Ad-hoc testing - Deploy changes and test manually
Integration tests

…loud Metrics usage

packages/api/tests/lambdas/sf-event-sqs-to-db-records/test-write-pdr.js

jennyhliu · 2025-01-08T21:54:47Z

packages/api/tests/lambdas/test-cleanExecutions.js

Maybe we can rename this file with different extension since we still want cleanExecution functionality, and will be updated in a different ticket.

jennyhliu · 2025-01-08T22:01:46Z

packages/api/lambdas/cleanExecutions.js

Suggest to comment out the code, and put a dummy handler, so the lambda can still be created in tf-modules/archive/clean_executions.tf

packages/api/lambdas/bootstrap.js

jennyhliu · 2025-01-09T17:29:56Z

packages/aws-client/tests/test-services.js

@@ -188,6 +188,7 @@ test('ec2() service defaults to localstack in test mode', async (t) => {
  );
 });

+// can this test be removed or is it needed for the granule CloudMetrics queries?


With es instance going away, I think packages/aws-client/tests/services.js es and this test are not needed.

packages/api/lambdas/create-reconciliation-report.js

jennyhliu · 2025-01-09T17:40:13Z

packages/api/tests/lambdas/test-bulk-operation.js

@@ -49,7 +49,7 @@ const esSearchStub = sandbox.stub();
 const esScrollStub = sandbox.stub();
 FakeEsClient.prototype.scroll = esScrollStub;
 FakeEsClient.prototype.search = esSearchStub;
-
+// is this es-client stuff still needed b/c granules and metrics?


I think es-client is still needed. You can try to take it out.

…d of full removal

…da and test code

npauzenga · 2025-01-13T17:14:16Z

CHANGELOG.md

@@ -92,6 +92,8 @@ External tooling making use of `searchContext` in the `GET` `/granules/` endpoin
  - Updated `@cumulus/api/bin/serveUtils` to no longer add records to ElasticSearch
  - Removed ElasticSearch from local API server code
  - Updated CollectionSearch to filter granule fields in addition to time frame for active collections
+- **CUMULUS-3847**
+  - list changes here


placeholder, I'm still making sure I've captured everything. will make sure this is filled out when I'm fairly certain

…files

npauzenga · 2025-01-13T17:16:13Z

packages/api/lambdas/bulk-operation.js

@@ -94,7 +94,7 @@ async function applyWorkflowToGranules({
 * @param {number} [payload.concurrency]
 *   granule concurrency for the bulk deletion operation.  Defaults to 10
 * @param {Object} [payload.query] - Optional parameter of query to send to ES
- * @param {string} [payload.index] - Optional parameter of ES index to query.
+ * @param {string} [payload.index] - Optional parameter of ES index (metrics) to query.


Meaning we're checking the Cloud Metrics ES index?

Yes, my understanding of why we need to keep the ES related functionality is that for some of the granules and bulk operations it will still query Cloud Metrics for data. I don't feel like totally understand how it all ties together, but that is what is mentioned in code and what I was informed of. So I thought it would be helpful to clarify as much as possible that the ES indexing and querying is going to Cloud Metrics and that's why it is staying around and not being removed as part of ES removal.

npauzenga · 2025-01-13T17:16:29Z

packages/api/lambdas/cleanExecutions.js

 const Logger = require('@cumulus/logger');
-const { sleep } = require('@cumulus/common');
+// const { sleep } = require('@cumulus/common');


Remove comments

We should avoid having commented-out code generally. We can rely on the git diff if we need to reference what was here.

hah. Well, I completely removed the lambda and test, and @jennyhliu suggested that I add them back and comment out except for a dummy handler. So I'm open to suggestions on why one method is better than the other...

(see comment at top of file)

If we remove lambda and test here, we have to remove the related tf code, that's why I suggested to have dummy handler since we want to keep the lambda with RDS implementation.
Now I think we can delete the code and leave only the dummy handler.

Ok, I can remove the commented code, if everyone is in agreement there.
What about the test file, should I remove the file altogether? Or if not, what should I leave in the file if not commented out tests?

npauzenga · 2025-01-13T17:19:51Z

packages/api/lib/testUtils.js

@@ -620,20 +620,14 @@ const createAsyncOperationTestRecords = async (context) => {
  };
 };

+// this doesn't look like it's being used anywhere, should I remove it?


If it's not used, I'd say yes. It's an exported member so technically (possibly) a user-facing change for those using this package but... it's only test utils.

npauzenga · 2025-01-13T17:20:40Z

packages/async-operations/tests/test-async_operations.js

@@ -61,7 +61,7 @@ test.before(async (t) => {
  t.context.functionConfig = {
    Environment: {
      Variables: {
-        ES_HOST: 'es-host',
+        Timeout: 300,


What's this for?

Since we're moving away from ES in general, ES indexing and all that, in the async operations tests I've changed all the places we're referencing ES operations to other options in those enums. Since we should probably be doing operations other than ES ones if ES is going away.

So just as I changed the operationTypes away from ES Index, I'm trying to move away from the ES_HOST. I wanted to keep a variable in here and found that Timeout could be one that is used.

npauzenga · 2025-01-13T17:20:57Z

packages/async-operations/tests/test-async_operations.js

@@ -357,7 +357,7 @@ test('getLambdaEnvironmentVariables returns expected environment variables', (t)
  const vars = getLambdaEnvironmentVariables(t.context.functionConfig);

  t.deepEqual(new Set(vars), new Set([
-    { name: 'ES_HOST', value: 'es-host' },
+    { name: 'Timeout', value: 300 },


Same as above, why this change?

npauzenga · 2025-01-13T17:21:29Z

tf-modules/archive/reconciliation_report.tf

@@ -15,8 +15,6 @@ resource "aws_lambda_function" "create_reconciliation_report" {
      CMR_HOST                         = var.cmr_custom_host
      DISTRIBUTION_ENDPOINT            = var.distribution_url
      ES_HOST                          = var.elasticsearch_hostname


What about ES_HOST?

So I removed these two variables in reference to the comment at the very end of packages/api/lambdas/create-reconciliation-report.js, where I also removed them from logging.
I will be writing a ticket to remove ES from all the other terraform files where they're not needed (minus Cloud Metrics) so I thought this should be part of that removal.

CUMULUS-3847: first round of changes, remove es indexing except for C…

bace254

…loud Metrics usage

charleshuang80 added work in progress high priority release-critical labels Jan 8, 2025

CUMULUS-3847: add changelog placeholder

a71fb8a

jennyhliu reviewed Jan 8, 2025

View reviewed changes

charleshuang80 added 3 commits January 9, 2025 10:33

CUMULUS-3847: fix lint issues, more index removal

f405651

CUMULUS-3874: add a few questions as comments

90683a9

CUMULUS-3847: more lint fixes

c6f3cee

jennyhliu reviewed Jan 9, 2025

View reviewed changes

charleshuang80 added 2 commits January 9, 2025 16:03

CUMULUS-3847: more removals, add back lambda to modify/comment instea…

5801e86

…d of full removal

CUMULUS-3847: remove lambda exports, comment out cleanExecutions lamb…

2b6cb9f

…da and test code

charleshuang80 added needs review and removed work in progress labels Jan 13, 2025

npauzenga reviewed Jan 13, 2025

View reviewed changes

CUMULUS-3847: more removals. try to disable max-len on commented out …

af84342

…files

npauzenga reviewed Jan 13, 2025

View reviewed changes

CUMULUS-3847: remove tf for removed bootstrap lambda

57df04d

npauzenga reviewed Jan 13, 2025

View reviewed changes

npauzenga added pr feedback and removed needs review labels Jan 13, 2025

charleshuang80 added 5 commits January 13, 2025 13:58

CUMULUS-3847: modify eslint rule disable

8034a5d

CUMULUS-3847: remove unused var

596ab38

CUMULUS-3847: remove resource reference

5317873

CUMULUS-3847: remove extra line

a1a5183

Merge branch 'feature/es-phase-2' into CUMULUS-3847

94b1b25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CUMULUS-3847: Remove any remaining ES indexing functions and tests #3897

CUMULUS-3847: Remove any remaining ES indexing functions and tests #3897

charleshuang80 commented Jan 8, 2025

jennyhliu Jan 8, 2025

jennyhliu Jan 8, 2025

jennyhliu Jan 9, 2025

jennyhliu Jan 9, 2025

npauzenga Jan 13, 2025

charleshuang80 Jan 13, 2025

npauzenga Jan 13, 2025

charleshuang80 Jan 13, 2025

npauzenga Jan 13, 2025

npauzenga Jan 13, 2025

charleshuang80 Jan 13, 2025

charleshuang80 Jan 13, 2025

jennyhliu Jan 13, 2025

charleshuang80 Jan 14, 2025

npauzenga Jan 13, 2025

npauzenga Jan 13, 2025

charleshuang80 Jan 13, 2025

charleshuang80 Jan 13, 2025

npauzenga Jan 13, 2025

npauzenga Jan 13, 2025

charleshuang80 Jan 13, 2025

CUMULUS-3847: Remove any remaining ES indexing functions and tests #3897

Are you sure you want to change the base?

CUMULUS-3847: Remove any remaining ES indexing functions and tests #3897

Conversation

charleshuang80 commented Jan 8, 2025

Changes

PR Checklist

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

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