[DSM] PEPPER-1402 PEPPER-1397 refactoring DSM event transmission into DSS and retrying failed events #2874

pegahtah · 2024-05-23T13:16:02Z

Context

Update 6/3

In the last commits I have made a super class BaseKitTestUtil (here) and two children classes ( PepperKitUtil and JuniperKitUtil ). I moved what was the base of both classes into the super class and created the two smaller classes

Original PR Context

This PR refactors the DSM event transmission process to DSS and implements a retry mechanism for failed events. Key changes include:

Consolidation of EventUtil into a new EventService.
Introduction of configurable retries with a backoff strategy for failed events in here
Cleanup of unused code and classes., For example deleted some TestBoston classes and events like ( TestBostonUPSTrackingJob and TestResultEvent
Enhancements in code readability and maintainability with added JavaDoc comments and better naming conventions, for example changed ParticipantEventRoute to SkippedParticipantEventRoute and created its own SkippedParticipantEventDao and SkippedParticipantEventDto, moved more functionality into EventDao
Improved error handling and logging for better monitoring and debugging. (see these parts of code)
Adding tests for various scenarios in event transmission in EventServiceTest

Checklist

I have labeled the type of changes involved using the C-* labels.
I have assessed potential risks and labeled using the R-* labels.
I have considered error handling and alerts, and added L-* labels as needed.
I have considered security and privacy, and added I-* labels as needed
I have analyzed my changes for stability, fault tolerance, graceful degradation, performance bottlenecks and written a brief summary in this PR.
If applicable, I have discussed the analytics needs at both a platform and study level with Product and instrumented code accordingly.
If applicable, my UI/UX changes have passed muster with Product/Design via an over-the-shoulder review, screenshots, etc.

If unsure or need help with any of the above items, add the help wanted label. For items that starts with If applicable, if it is not applicable, check it off and add n/a in front.

FUD Score

Overall, how are you feeling about these changes?

☺️ All good, business as usual!
😅 There might be some issues here
😱 I'm sweaty and nervous

How do we demo these changes?

How does one observe these changes in a deployed system? Note that user visible encompasses many personas--not just patients and study staff, but also ops duty, your fellow devs, compliance, etc.

They are user-visible in dev as a regular user journey and require no additional instructions.
Getting dev into a state where this is user-visible requires some tech fiddling. I have documented these steps in the related ticket.
Requires other features before it's human visible. I have documented the blocking issues in jira.
I have no idea how to demo this. Please help me!

Testing

I have added regression test labels to the jira ticket
I have added verification steps to the jira ticket
I have written automated positive tests
I have written automated negative tests
I have written zero automated tests but have poked around locally to verify proper functionality
The jira ticket has acceptance criteria and QA has the needed information to test changes

Release

These changes require no special release procedures--just code!
Releasing these changes requires special handling and I have documented the special procedures in the release plan document

…-1397_trigger_ddp

added retries removed connections renamed and made the code more readable got rid of unnecessary codes and classes

some clean up

some more refactoring

renaming some classes

# Conflicts: # pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/kits/TestKitUtil.java

…ass name to EventService and moved it to service, also changed the name of the test class and moved it,

…-1397_trigger_ddp

pegahtah · 2024-05-23T16:58:15Z

pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/DDPInstance.java

@@ -404,31 +404,6 @@ public static DDPInstance getDDPInstanceWithRoleByParticipant(@NonNull String pa
        return (DDPInstance) results.resultValue;
    }

-    public static boolean getRole(@NonNull String realm, @NonNull String role) {


Was not used

…ao classes to the right packages, also added Mockito.CALLS_REAL_METHODS to make tests more readable

pegahtah · 2024-05-23T17:58:45Z

pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/DDPInstance.java

@@ -276,7 +276,7 @@ public static DDPInstance getDDPInstanceWithRole(@NonNull String realm, @NonNull
    public static DDPInstance getDDPInstanceWithRoleByStudyGuid(@NonNull String studyGuid, @NonNull String role) {
        SimpleResult results = inTransaction((conn) -> {
            SimpleResult dbVals = new SimpleResult();
-            try (PreparedStatement stmt = conn.prepareStatement(SQL_SELECT_INSTANCE_WITH_ROLE + QueryExtension.BY_STUDY_GUID)) {
+            try (PreparedStatement stmt = conn.prepareStatement(SQL_SELECT_INSTANCE_WITH_ROLE.concat(QueryExtension.BY_STUDY_GUID))) {


the concat changes are a fix for sonar marking these as "blocker" issues

pegahtah · 2024-05-23T18:02:17Z

pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/jobs/TestBostonUPSTrackingJob.java

@@ -1,610 +0,0 @@
-package org.broadinstitute.dsm.jobs;


Got rid of this job

pegahtah · 2024-05-23T18:02:42Z

pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/TestResultEvent.java

@@ -1,15 +0,0 @@
-package org.broadinstitute.dsm.model;


We also don't need this event anymore

pegahtah · 2024-05-23T18:03:05Z

pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/gp/BSPKit.java

+            + " realm.migrated_ddp, kit.receive_date, kit.scan_date from ddp_kit_request request, ddp_kit kit, event_type eve, "
+            + " ddp_instance realm where request.dsm_kit_request_id = kit.dsm_kit_request_id and "
+            + " request.ddp_instance_id = realm.ddp_instance_id and (eve.ddp_instance_id = request.ddp_instance_id "
+            + " and eve.kit_type_id = request.kit_type_id) and eve.event_type = 'RECEIVED' and kit.kit_label = ?";


moved this from the conf file to the class for better testing and code readability

pegahtah · 2024-05-23T18:03:54Z

pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/model/gp/GPReceivedKit.java

@@ -82,18 +82,18 @@ public static Optional<KitInfo> receiveKit(String kitLabel, BSPKitDto bspKitQuer

    }

-    private static void updateKitAndExport(String kitLabel, BSPKitDao bspKitDao, BSPKitDto maybeBspKitQueryResult, boolean triggerDDP,


The maybe in the name was throwing me off and made we think we have an optional here, so I removed it

pegahtah · 2024-05-23T18:13:47Z

pepper-apis/dsm-core/src/main/java/org/broadinstitute/lddp/handlers/util/Event.java

@@ -1,21 +0,0 @@
-package org.broadinstitute.lddp.handlers.util;


Merged this and the KitEvent class into a new Event class because this original Event class was not used anywhere by its own, the only usage was that KitEvent inherited from it

pegahtah · 2024-05-23T18:14:09Z

pepper-apis/dsm-core/src/main/resources/liquibase/PEPPER-1397_instance_role_insert.xml

@@ -0,0 +1,19 @@
+<?xml version="1.0" encoding="UTF-8"?>


This role was needed in the test db for testing purposes

pegahtah · 2024-05-23T18:15:59Z

pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/juniperkits/JuniperTestSetupUtil.java

@@ -15,6 +15,7 @@
 import java.util.List;


Changes here are not really related to this ticket, just some clean up that came up when I was making changes in the TestKitUtil.

pegahtah · 2024-05-23T18:43:00Z

pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dto/queue/EventDto.java

 public class EventDto {

    private int eventId;
    private long eventDateCreated;
    private String eventType;
    private int ddpInstanceId;
-    private int dsmKitRequestId;
+    private Integer dsmKitRequestId;


for participant events the dsmKitRequestId will be null

Worth putting this into a code comment!

pegahtah · 2024-05-23T18:45:38Z

...er-apis/dsm-core/src/main/java/org/broadinstitute/dsm/pubsub/MercuryOrderStatusListener.java

 import org.broadinstitute.dsm.model.mercury.BaseMercuryStatusMessage;

 @Slf4j
 public class MercuryOrderStatusListener {

-    private MercuryOrderStatusListener(){


Checkstyle fixes here

pegahtah · 2024-05-23T19:03:54Z

pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/util/EventUtil.java

@@ -1,276 +0,0 @@
-package org.broadinstitute.dsm.util;


This has been changed to EventService

zyme · 2024-05-31T11:54:13Z

pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/kits/TestKitUtil.java

+                }
+                stmt.setString(5, eventType);
+                stmt.executeUpdate();
+            } catch (SQLException e) {


I think there's an antipattern here--the method returns void, but there's this two-step try/catch and then a check of results.resultException. To reduce the complexity, instead of handling the return value from inTransaction this way, just do what you need to do within inTransaction, returning null, and then catch SQLException and retrhwo as DsmInternalError.

zyme · 2024-05-31T12:04:39Z

pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/EventService.java

+        RetryConfig retryConfig = RetryConfig.custom()
+                .maxAttempts(MAX_TRIES)
+                .intervalFunction(intervalFn)
+                .build();


I think the initial interval needs to be longer. What are the typical response times when hitting this DSS endpoint? If the response time is shorter than the typical time it takes for the dss route to complete, does this run the risk of making repeated attempts that will actually get through, then causing further issues?

…r a response from dss , and also did a major refactoring for our util classes in test that were creating test kits.

…-1397_trigger_ddp # Conflicts: # pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dao/kit/KitDao.java

pegahtah · 2024-06-03T19:25:15Z

pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/juniperkits/JuniperTestSetupUtil.java

@@ -1,339 +0,0 @@
-package org.broadinstitute.dsm.juniperkits;


Was renamed and refactored into JuniperKitUtil

pegahtah · 2024-06-03T19:27:19Z

pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/kits/BaseKitTestUtil.java

@@ -0,0 +1,264 @@
+package org.broadinstitute.dsm.kits;


This class consists of the overlapped functionality that used to be in the TestKitUtil and JuniperTestSetupUtil.
In the new structure this class contains what is a standard kit's journey and specific kit setup that belongs to only one of Juniper or Pepper is now in JuniperKitUtil and PepperKitUtil

pegahtah · 2024-06-03T19:42:30Z

pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/kits/PepperKitUtil.java

-            throw new DsmInternalError(String.format("Unable to get primary key for %s", table));
-        }
-    }
-


All moved to super class

pegahtah · 2024-06-03T19:43:09Z

pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/kits/PepperKitUtil.java

-        stmt.executeUpdate();
-        ResultSet rs = stmt.getGeneratedKeys();
-        return getPrimaryKey(rs, "kit_dimension");
-    }


All moved into super class

pegahtah · 2024-06-03T19:43:55Z

pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/kits/PepperKitUtil.java

-
-        return new Gson().fromJson(json, JuniperKitRequest.class);
-    }
-


Not relevant to PepperKitUtil

pegahtah · 2024-06-03T19:50:52Z

.../test/java/org/broadinstitute/dsm/service/adminoperation/UpdateKitToLegacyIdServiceTest.java

@@ -11,7 +11,7 @@
 import org.broadinstitute.dsm.db.dao.ddp.instance.DDPInstanceDao;


Only change in this class is name changes of TestKitUtil to PepperKitUtil

pegahtah · 2024-06-03T19:52:00Z

pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/service/admin/UserAdminTestUtil.java

@@ -107,7 +107,7 @@ public void createRealmAndStudyGroup(@NonNull String realmName, String studyGuid
     * Setup study admin and study roles
     *
     * @param adminEmail    for new admin user account
-     * @param adminRole     PEPPER_ADMIN or STUDY_USER_ADMIN


PEPPER_ADMIN_ROLE is the correct one

pegahtah · 2024-06-03T19:55:54Z

pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/kits/KitDisplayNameTest.java

@@ -3,40 +3,55 @@
 import java.util.List;

 import org.broadinstitute.dsm.DbTxnBaseTest;


This class was changes a little bit because it was using juniper kits while it should have been pepper kits. Now that we have two functionalities for creating kits, I changed this to use the correct one (otherwise tests would not pass)

pegahtah · 2024-06-03T20:01:36Z

pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/kits/PepperKitUtil.java

@@ -3,9 +3,7 @@
 import static org.broadinstitute.ddp.db.TransactionWrapper.inTransaction;


TestKitUtil is not changed to PepperKitUtil, while lots of its kit functionality is now in the BaseKitTestUtil

pegahtah · 2024-06-03T20:02:51Z

.../dsm-core/src/test/java/org/broadinstitute/dsm/juniperkits/JuniperKitCreationStatusTest.java

@@ -30,6 +30,7 @@
 import org.broadinstitute.dsm.model.nonpepperkit.NonPepperStatusKitService;


the changes here are only name changes from JuniperTestSetupUtil to JuniperKitUtil

pegahtah · 2024-06-03T20:03:40Z

...-apis/dsm-core/src/test/java/org/broadinstitute/dsm/db/dao/mercury/ClinicalOrderDaoTest.java

@@ -64,7 +64,7 @@ public class ClinicalOrderDaoTest extends DbTxnBaseTest {

    private static int oncHistoryDetailId = -1;

-    public static TestKitUtil testKitUtil;


That's basically the only change in this class, and below is only the name changed for pepperKitUtil

pegahtah · 2024-06-03T20:03:51Z

pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/service/EventService.java

@@ -96,7 +96,7 @@ protected static boolean triggerDssByKitEvent(KitDDPNotification kitDDPNotificat
    @VisibleForTesting
    protected static boolean triggerDssWithEvent(@NonNull String eventType, DDPInstance ddpInstance, long eventDate,
                                        @NotNull String ddpParticipantId, @NotNull String eventInfo, KitReasonType reason) {
-        final long initialInterval = 100; // base delay in milliseconds


I increased the initial interval

pegahtah · 2024-06-03T20:16:59Z

pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/juniperkits/JuniperKitUtil.java

+ */
+@Slf4j
+@Getter
+public class JuniperKitUtil extends BaseKitTestUtil {


This class has the config setup and what else that is only related to Juniper kits

…erKitUtil.java and JuniperKitUtil.java

zyme · 2024-06-06T11:45:40Z

pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/kits/KitTestUtil.java

+    protected Integer kitDimensionId;
+    protected Integer kitReturnId;
+    protected  Integer carrierId;
+    protected Integer ddpKitRequestSettingsId;


These don't need to be protected now that there's not a base class. I think they can all be private.

…-1397_trigger_ddp

pegahtah added 18 commits April 10, 2024 21:48

PEPPER-1402 WIP some refactoring and test working now

b75f783

Merge remote-tracking branch 'origin/develop' into PEPPER-1402_PEPPER…

8f7ce29

…-1397_trigger_ddp

Merge branch 'develop' into PEPPER-1402_PEPPER-1397_trigger_ddp

864f014

PEPPER-1402 PEPPER-1397

4e39668

added retries removed connections renamed and made the code more readable got rid of unnecessary codes and classes

PEPPER-1402 PEPPER-1397

962e9f6

some clean up

PEPPER-1402 PEPPER-1397

d0d09f8

some more refactoring

PEPPER-1402 PEPPER-1397

ef86513

renaming some classes

PEPPER-1402 PEPPER-1397

5aca459

renaming some classes

PEPPER-1402 merged develop in and added changes

b9aba34

PEPPER-1402 checkstyle fix

a91eb51

Merge branch 'develop' into PEPPER-1402_PEPPER-1397_trigger_ddp

2011496

# Conflicts: # pepper-apis/dsm-core/src/test/java/org/broadinstitute/dsm/kits/TestKitUtil.java

PEPPER-1402 more refactoring, changed EventUtil to EventService

c392262

PEPPER-1402 PEPPER-1397 added tests for all scenarios, changed the cl…

73192f6

…ass name to EventService and moved it to service, also changed the name of the test class and moved it,

PEPPER-1402 PEPPER-1397 checkstyle fix

5c40fa7

Merge remote-tracking branch 'origin/develop' into PEPPER-1402_PEPPER…

b483a7f

…-1397_trigger_ddp

PEPPER-1402 PEPPER-1397 deleting unused classes

2d31cc2

PEPPER-1402 PEPPER-1397 deleting unused classes

4114d03

PEPPER-1402 PEPPER-1397 small clean ups

71acc96

pegahtah commented May 23, 2024

View reviewed changes

pegahtah added 4 commits May 23, 2024 13:57

PEPPER-1402 PEPPER-1397 fixed a bug in TestHelper and moved dto and d…

a68831a

…ao classes to the right packages, also added Mockito.CALLS_REAL_METHODS to make tests more readable

PEPPER-1402 PEPPER-1397 fixed java doc comment

9a33f71

PEPPER-1402 PEPPER-1397 small letters

90db874

PEPPER-1402 PEPPER-1397 small letters

f41e052

pegahtah commented May 23, 2024

View reviewed changes

pegahtah requested a review from zyme May 23, 2024 19:32

pegahtah added C-bug Category: identifies or fixes a bug C-feature Category: non-breaking change which adds new functionality C-enhancement Category: code clean ups, refactors, or general enhancements java Pull requests that update Java code labels May 23, 2024

zyme reviewed May 31, 2024

View reviewed changes

pegahtah added 2 commits June 3, 2024 13:33

PEPPER-1402 based on PR comments I increased the initial wait time fo…

3b72df0

…r a response from dss , and also did a major refactoring for our util classes in test that were creating test kits.

Merge remote-tracking branch 'origin/develop' into PEPPER-1402_PEPPER…

4c27130

…-1397_trigger_ddp # Conflicts: # pepper-apis/dsm-core/src/main/java/org/broadinstitute/dsm/db/dao/kit/KitDao.java

pegahtah commented Jun 3, 2024

View reviewed changes

PEPPER-1402 cleaned up code

5158814

pegahtah commented Jun 3, 2024

View reviewed changes

pegahtah added 2 commits June 3, 2024 15:44

PEPPER-1402 cleaned up code

81336d8

PEPPER-1402 cleaned up code

66308da

pegahtah commented Jun 3, 2024

View reviewed changes

PEPPER-1402 made BaseKitTestUtil an abstract class

f5834d3

pegahtah requested a review from zyme June 5, 2024 15:57

PEPPER-1402 changed BaseKitTestUtil into KitTestUtil and removed Pepp…

d8dd384

…erKitUtil.java and JuniperKitUtil.java

zyme reviewed Jun 6, 2024

View reviewed changes

zyme approved these changes Jun 6, 2024

View reviewed changes

pegahtah and others added 3 commits June 6, 2024 14:10

Merge branch 'develop' into PEPPER-1402_PEPPER-1397_trigger_ddp

c3dbf1c

Merge remote-tracking branch 'origin/develop' into PEPPER-1402_PEPPER…

66ddffb

…-1397_trigger_ddp

PEPPER-1402 resolving conflicts

dad9982

pegahtah merged commit 28f95a6 into develop Jun 6, 2024
8 checks passed

pegahtah deleted the PEPPER-1402_PEPPER-1397_trigger_ddp branch June 6, 2024 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DSM] PEPPER-1402 PEPPER-1397 refactoring DSM event transmission into DSS and retrying failed events #2874

[DSM] PEPPER-1402 PEPPER-1397 refactoring DSM event transmission into DSS and retrying failed events #2874

pegahtah commented May 23, 2024 •

edited

Loading

pegahtah May 23, 2024

pegahtah May 23, 2024

pegahtah May 23, 2024

pegahtah May 23, 2024

pegahtah May 23, 2024

pegahtah May 23, 2024

pegahtah May 23, 2024

pegahtah May 23, 2024

pegahtah May 23, 2024

pegahtah May 23, 2024

zyme May 24, 2024

pegahtah May 23, 2024

pegahtah May 23, 2024

zyme May 31, 2024

zyme May 31, 2024

pegahtah Jun 3, 2024

pegahtah Jun 3, 2024 •

edited

Loading

pegahtah Jun 3, 2024

pegahtah Jun 3, 2024

pegahtah Jun 3, 2024

pegahtah Jun 3, 2024

pegahtah Jun 3, 2024

pegahtah Jun 3, 2024

pegahtah Jun 3, 2024

pegahtah Jun 3, 2024

pegahtah Jun 3, 2024

pegahtah Jun 3, 2024

pegahtah Jun 3, 2024

zyme Jun 6, 2024

		@@ -82,18 +82,18 @@ public static Optional<KitInfo> receiveKit(String kitLabel, BSPKitDto bspKitQuer

		}

		private static void updateKitAndExport(String kitLabel, BSPKitDao bspKitDao, BSPKitDto maybeBspKitQueryResult, boolean triggerDDP,

		@@ -1,21 +0,0 @@
		package org.broadinstitute.lddp.handlers.util;

		@@ -1,339 +0,0 @@
		package org.broadinstitute.dsm.juniperkits;

		@@ -11,7 +11,7 @@
		import org.broadinstitute.dsm.db.dao.ddp.instance.DDPInstanceDao;

		@@ -3,40 +3,55 @@
		import java.util.List;

		import org.broadinstitute.dsm.DbTxnBaseTest;

		@@ -3,9 +3,7 @@
		import static org.broadinstitute.ddp.db.TransactionWrapper.inTransaction;

		@@ -30,6 +30,7 @@
		import org.broadinstitute.dsm.model.nonpepperkit.NonPepperStatusKitService;

		@@ -64,7 +64,7 @@ public class ClinicalOrderDaoTest extends DbTxnBaseTest {

		private static int oncHistoryDetailId = -1;

		public static TestKitUtil testKitUtil;

[DSM] PEPPER-1402 PEPPER-1397 refactoring DSM event transmission into DSS and retrying failed events #2874

[DSM] PEPPER-1402 PEPPER-1397 refactoring DSM event transmission into DSS and retrying failed events #2874

Conversation

pegahtah commented May 23, 2024 • edited Loading

Context

Update 6/3

Original PR Context

Checklist

FUD Score

How do we demo these changes?

Testing

Release

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

pegahtah Jun 3, 2024 • 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

Choose a reason for hiding this comment

pegahtah commented May 23, 2024 •

edited

Loading

pegahtah Jun 3, 2024 •

edited

Loading