Harmony 1996: Add 'empty-result' status to WorkItem class and work-items table #683

indiejames · 2025-01-23T15:59:37Z

Jira Issue ID

HARMONY-1996

Description

This PR adds a new status to WorkItem to represent when a service processed the work-item, but produced to data. This is to allow us to provide a better experience for users by not staging empty files.

Local Test Steps

Use dbViz to update a work-item's status to 'empty-result' and verify that this is allowed. (This change will not be visible anywhere until further tickets are worked)
Run a Harmony request and verify that it goes through okay.
Check the status page and workflow UI to make sure the behavior has not changed

PR Acceptance Checklist

Acceptance criteria met
Tests added/updated (if needed) and passing
~~Documentation updated (if needed)~~
[ x] Harmony in a Box tested? (if changes made to microservices)

and level/category to job_errors to support warnings as well as errors

ygliuvt · 2025-01-24T20:48:56Z

db/db.sql

@@ -80,7 +92,8 @@ CREATE TABLE `work_items` (
  `workflowStepIndex` integer not null,
  `scrollID` varchar(4096),
  `serviceID` varchar(255) not null,
-  `status` text check (`status` in ('ready', 'queued', 'running', 'successful', 'failed', 'canceled')) not null,
+  `status` varchar(255) check (`status` in ('ready', 'queued', 'running', 'successful', 'failed', 'canceled', 'warning')) not null,
+  `subStatus` varchar(255) check (`subStatus` in ('no-data')),


The column name should besub_status to match the Postgres definition.

The value is missing null.

ygliuvt · 2025-01-24T20:49:14Z

db/db.sql

 CREATE INDEX work_items_jobID_idx ON work_items(jobID);
 CREATE INDEX work_items_serviceID_idx ON work_items(serviceID);
 CREATE INDEX work_items_status_idx ON work_items(status);
+CREATE INDEX work_items_subStatus_idx ON work_items(subStatus);


The column name should besub_status to match the Postgres definition.

vinnyinverso · 2025-01-24T20:58:08Z

db/db.sql

+  )
+  ) not null default 'error',
+  `category` varchar(255) check (
+    `category` in (


Do we want to have to change this check every time a new category is added in service lib? If we're adding new categories only via pull request in the service lib, it has been validated already by the time it gets to Harmony. This is also missing the existing error categories like "Server", "Forbidden".

That's a good point. I'll drop this constraint here as well as in the migration.

ygliuvt · 2025-01-24T21:03:00Z

services/harmony/app/models/work-item.ts

@@ -51,6 +51,9 @@ export default class WorkItem extends Record implements WorkItemRecord {
  // The status of the operation - see WorkItemStatus
  status?: WorkItemStatus;

+  // The sub-status of the operation - see WorkItemSubStatus
+  subStatus?: WorkItemSubStatus;


nit: I think we have coding standard to use underscores in new column names. i.e. sub_status.
I commented above that the sub_status name is not consistent in db definition vs migration. If you want to go with subStatus, it is fine with me as long as we make it consistent.

For some reason I thought it could be snake case in the dB, but camel case in the models. I see now that we are not doing that (maybe it doesn't work). I'll make it snake case here.

db/migrations/20250122190038_add_warning_status_and_sub_status_to_work_items.js

sub_status field to be consistent

chris-durbin

I have a general question about sub_status and category. I see in work_items we are calling the field sub_status and in job_errors we're calling it category. I believe they are the same thing - would it be better to use the same column name?

chris-durbin · 2025-01-27T10:30:29Z

db/migrations/20250122190038_add_warning_status_and_sub_status_to_work_items.js

+    CHECK (status IN ('ready', 'queued', 'running', 'successful', 'failed', 'canceled', 'warning')),
+    ADD COLUMN "sub_status" VARCHAR(255),
+    ADD CONSTRAINT "work_items_sub_status_check"
+    CHECK (sub_status IN (null, 'no-data'));


I don't think we should have an enumeration on this field which I think we were calling "category" - we should allow any string and don't need this constraint.

chris-durbin · 2025-01-27T10:32:23Z

db/migrations/20250124155544_add_level_and_category_to_job_errors.js

+    ADD COLUMN "category" VARCHAR(255) DEFAULT 'generic' NOT NULL;
+
+    CREATE INDEX job_errors_level ON job_errors (level);
+    CREATE INDEX job_errors_category ON job_errors (category)


I'm not sure if it matters, but for consistency should we have each statement end in a semi-colon?

chris-durbin · 2025-01-27T10:33:39Z

db/migrations/20250124155544_add_level_and_category_to_job_errors.js

+    DROP INDEX job_errors_category;
+    DROP_INDEX job_errors_level;
+    ALTER TABLE "job_errors"
+    DROP CONSTRAINT "job_errors_category_check",


I don't see a job_error_category_check constraint in the up migration - and I don't think we want one so assume this should be removed.

chris-durbin · 2025-01-27T10:34:23Z

packages/util/tsconfig.build.json

@@ -10,7 +10,10 @@
      "allowJs": true,
      "noImplicitAny": false,
      "sourceMap": true,
-      "outDir": "built"
+      "outDir": "built",
+      "typeRoots": [


Out of curiosity what does this fix - anything we should do differently?

I was having consistent failures from tsc complaining about not finding types for lots of packages. This tells it where to look. Not sure why no one else has seen these errors.

ygliuvt · 2025-01-27T16:20:16Z

db/migrations/20250122190038_add_warning_status_and_sub_status_to_work_items.js

+    CHECK (status IN ('ready', 'queued', 'running', 'successful', 'failed', 'canceled', 'warning')),
+    ADD COLUMN "sub_status" VARCHAR(255);
+
+    CREATE INDEX work_items_sub_status ON work_items (sub_status);


nit: add _index to index name for consistency. i.e. work_items_sub_status_index.

ygliuvt · 2025-01-27T16:34:27Z

db/migrations/20250124155544_add_level_and_category_to_job_errors.js

+    CHECK (level IN ('error', 'warning')),
+    ADD COLUMN "category" VARCHAR(255);
+
+    CREATE INDEX job_errors_level ON job_errors (level);


same comment with index naming convention.

ygliuvt · 2025-01-27T16:35:10Z

db/migrations/20250124155544_add_level_and_category_to_job_errors.js

+ */
+exports.down = function (knex) {
+  return knex.schema.raw(`
+    DROP INDEX job_errors_category;


I got - syntax error at or near "DROP_INDEX" error when running the down migration.

ugh - fixed here and in the other migration

ygliuvt · 2025-01-27T16:35:42Z

db/migrations/20250124155544_add_level_and_category_to_job_errors.js

+    ALTER TABLE "job_errors"
+    DROP COLUMN "category",
+    DROP CONSTRAINT "job_errors_level_check",
+    DROP COLUMN "level;"


typo in level;

ygliuvt

Need to fix the down migration issue.

indiejames · 2025-01-27T17:01:48Z

Need to fix the down migration issue.

Done

chris-durbin

Successfully tested the migrations and rollbacks. Verified requests continued to work as expected.

ygliuvt

Tested successfully.

indiejames added 2 commits January 22, 2025 16:23

HARMONY-1996: Add EMPTY_RESULT to WorkItemStatus

d6038fa

HARMONY-1996: Add new db migration file missed on previous commit

8ccb9d3

indiejames requested review from ygliuvt, chris-durbin and vinnyinverso January 23, 2025 15:59

indiejames added 4 commits January 23, 2025 14:38

HARMONY-1996: Change empty-result status to no-data

6ad13b8

HARMONY-1996: Changes to tsconfig.json files to fix tsc errors

4fe4a84

HARMONY-1996: Add work-item status 'warning' and substatus work items

8551b21

and level/category to job_errors to support warnings as well as errors

HARMONY-1996: Remove unnecessary semicolon

57ad87c

ygliuvt reviewed Jan 24, 2025

View reviewed changes

vinnyinverso reviewed Jan 24, 2025

View reviewed changes

ygliuvt reviewed Jan 24, 2025

View reviewed changes

vinnyinverso reviewed Jan 24, 2025

View reviewed changes

db/migrations/20250122190038_add_warning_status_and_sub_status_to_work_items.js Show resolved Hide resolved

HARMONY-1996: Remove unneeded dB constraint and fix casing on model

48b513b

sub_status field to be consistent

chris-durbin reviewed Jan 27, 2025

View reviewed changes

HARMONY-1996: Remove unneeded constrains on dB tables

a67ca1b

indiejames requested review from vinnyinverso, ygliuvt and chris-durbin January 27, 2025 15:17

ygliuvt reviewed Jan 27, 2025

View reviewed changes

ygliuvt requested changes Jan 27, 2025

View reviewed changes

HARMONY-1996: Fix down migrations

d105361

indiejames requested a review from ygliuvt January 27, 2025 17:01

chris-durbin approved these changes Jan 27, 2025

View reviewed changes

ygliuvt approved these changes Jan 27, 2025

View reviewed changes

indiejames merged commit 229aa98 into main Jan 27, 2025
5 checks passed

indiejames deleted the harmony-1996 branch January 27, 2025 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Harmony 1996: Add 'empty-result' status to WorkItem class and work-items table #683

Harmony 1996: Add 'empty-result' status to WorkItem class and work-items table #683

indiejames commented Jan 23, 2025

ygliuvt Jan 24, 2025

ygliuvt Jan 24, 2025

indiejames Jan 24, 2025

ygliuvt Jan 24, 2025

indiejames Jan 24, 2025

vinnyinverso Jan 24, 2025 •

edited

Loading

indiejames Jan 24, 2025

ygliuvt Jan 24, 2025

indiejames Jan 24, 2025

chris-durbin left a comment

chris-durbin Jan 27, 2025

indiejames Jan 27, 2025

chris-durbin Jan 27, 2025

indiejames Jan 27, 2025

chris-durbin Jan 27, 2025

indiejames Jan 27, 2025

chris-durbin Jan 27, 2025

indiejames Jan 27, 2025

ygliuvt Jan 27, 2025

indiejames Jan 27, 2025

ygliuvt Jan 27, 2025

indiejames Jan 27, 2025

ygliuvt Jan 27, 2025

indiejames Jan 27, 2025

ygliuvt Jan 27, 2025

indiejames Jan 27, 2025

ygliuvt left a comment •

edited

Loading

indiejames commented Jan 27, 2025

chris-durbin left a comment

ygliuvt left a comment

Harmony 1996: Add 'empty-result' status to WorkItem class and work-items table #683

Harmony 1996: Add 'empty-result' status to WorkItem class and work-items table #683

Conversation

indiejames commented Jan 23, 2025

Jira Issue ID

Description

Local Test Steps

PR Acceptance 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

vinnyinverso Jan 24, 2025 • 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

chris-durbin left a 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

ygliuvt left a comment • edited Loading

Choose a reason for hiding this comment

indiejames commented Jan 27, 2025

chris-durbin left a comment

Choose a reason for hiding this comment

ygliuvt left a comment

Choose a reason for hiding this comment

vinnyinverso Jan 24, 2025 •

edited

Loading

ygliuvt left a comment •

edited

Loading