Allow legacy deduplication #741

jh-bate · 2024-06-18T08:36:16Z

back-37 - Migrate all existing jellyfish data to add required Platform deduplication hash using the existing generated jellyfish _id
back-2773 - Update platform to validate certain fields in pumpSettingsOverride and overridePresets
back-2728 - Support tandem pumpSettings in the platform
back-2520 - Data model changes to support Control-IQ sleep schedules
BACK-2607 - Bolus includes deliveryContext
BACK-3074 - basal/suspend to remove the duration maximum validation

it has still been processed

tidebot · 2024-10-08T00:47:02Z

jh-bate updated values.yaml file in qa3

tidebot · 2024-10-08T00:47:03Z

jh-bate updated flux policies file in qa3

tidebot · 2024-10-08T00:47:04Z

jh-bate deployed platform jf_deduplication branch to qa3 namespace

jh-bate · 2024-10-08T02:26:44Z

/deploy qa3 data

tidebot · 2024-10-08T02:26:47Z

jh-bate updated values.yaml file in qa3

tidebot · 2024-10-08T02:26:48Z

jh-bate updated flux policies file in qa3

tidebot · 2024-10-08T02:26:49Z

jh-bate deployed platform jf_deduplication branch to qa3 namespace

darinkrauss

Lots of questions and comments, but it is a lot of code. :)

data/blood/glucose/test/glucose.go

dexcom/fetch/translate.go

data/types/common/day.go

data/types/common/day_test.go

data/datum.go

data/deduplicator/deduplicator/hash.go

darinkrauss · 2024-10-14T23:55:02Z

migrations/20231128_jellyfish_migration/utils/data_migration.go

+
+func (m *DataMigration) writeErrors() {
+	if !m.settings.writeToDisk {
+		m.groupedErrors = map[string][]ErrorData{}


Should this return here?

darinkrauss · 2024-10-14T23:55:56Z

migrations/20231128_jellyfish_migration/utils/data_migration.go

+	return nil
+}
+
+func (c MigrationStats) report() {


Maybe put this with the rest of MigrationStats?

darinkrauss · 2024-10-15T00:28:35Z

migrations/20231128_jellyfish_migration/utils/jellyfish_data.go

@@ -0,0 +1,175 @@
+package utils


What all are we doing here? Just adding the deduplicator name, version, and hash, right? Can we just replace this with a direct Mongo update?

yeah I am beginning to wonder if it is even required

after discussion with Jimmy and how he ran the test I don't think this is required any longer

Why don't you keep this code in a separate saved feature branch, just in case.

jh-bate · 2024-10-30T02:07:24Z

/deploy qa3 data

tidebot · 2024-10-30T02:07:29Z

jh-bate updated values.yaml file in qa3

tidebot · 2024-10-30T02:07:29Z

jh-bate updated flux policies file in qa3

tidebot · 2024-10-30T02:07:30Z

jh-bate deployed platform jf_deduplication branch to qa3 namespace

darinkrauss

A few more changes and clarifications. Plus, one thing we should chat about regarding the deduplication version versus the identity fields version.

darinkrauss · 2024-11-02T00:47:43Z

data/store/mongo/mongo_data_set.go

-
-	if filter.IsLegacy != nil && *filter.IsLegacy {
-		selector["_id"] = bson.M{"$not": bson.M{"$type": "objectId"}}
+	if filter.LegacyOnly != nil {
+		if *filter.LegacyOnly {
+			selector["_id"] = bson.M{"$not": bson.M{"$type": "objectId"}}
+		} else {
+			selector["_id"] = bson.M{"$type": "objectId"}
+		}


Sorry, it was either rename it to LegacyOnly OR change the code. I'd revert this code back to what it was deleted here.

data/types/settings/pump/sleep_schedule.go

darinkrauss · 2024-11-02T01:00:26Z

data/types/bolus/bolus.go

+	if b.DeliveryContext != nil {
+		validator.String("deliveryContext", b.DeliveryContext).Exists().OneOf(DeliveryContext()...)
+	}


This should just change to:

Suggested change

if b.DeliveryContext != nil {

validator.String("deliveryContext", b.DeliveryContext).Exists().OneOf(DeliveryContext()...)

}

validator.String("deliveryContext", b.DeliveryContext).OneOf(DeliveryContext()...)

There is no need for the b.DeliveryContext != nil. The validator handles that via the Exists test, which we also don't want because this field is optional.

darinkrauss · 2024-11-02T01:10:15Z

data/deduplicator/deduplicator/device_deactivate_hash.go

+	DeviceDeactivateHashVersionUnknown string = ""
+	DeviceDeactivateHashVersionCurrent string = types.IdentityFieldsVersion
+	DeviceDeactivateHashVersionLegacy  string = types.LegacyIdentityFieldsVersion


You actually don't need the string type as it is implicit.

Suggested change

DeviceDeactivateHashVersionUnknown string = ""

DeviceDeactivateHashVersionCurrent string = types.IdentityFieldsVersion

DeviceDeactivateHashVersionLegacy string = types.LegacyIdentityFieldsVersion

DeviceDeactivateHashVersionUnknown = ""

DeviceDeactivateHashVersionCurrent = types.IdentityFieldsVersion

DeviceDeactivateHashVersionLegacy = types.LegacyIdentityFieldsVersion

darinkrauss · 2024-11-02T01:13:35Z

data/deduplicator/deduplicator/device_deactivate_hash.go

+				if *dataSet.Deduplicator.Version != "" {
+					return *dataSet.Deduplicator.Version
+				}


You don't need this conditional. It is checked two lines above.

Suggested change

if *dataSet.Deduplicator.Version != "" {

return *dataSet.Deduplicator.Version

}

return *dataSet.Deduplicator.Version

Actually, I think may need to modify this. Since we are just returning the *dataSet.Deduplicator.Version here it is very possible is not either either the current or the legacy and the logic would break. I think we may need to break the intertwined DeviceDeactivateHashVersionLegacy and types.LegacyIdentityFieldsVersion.

Let's chat about this next week.

darinkrauss · 2024-11-02T01:21:08Z

data/deduplicator/deduplicator/hash.go

+	}
+}
+
+var missingLegacyGroupIdErr = errors.New("missing required legacy groupId for the device deactive hash legacy version")


We don't really want to create static error variables since it loses the context of where the error actually originated from (the caller). Just take the right side and replace it in the two locations of missingLegacyGroupIdErr.

darinkrauss · 2024-11-02T01:21:46Z

data/deduplicator/deduplicator/hash.go

+			return missingLegacyGroupIdErr
+		}
+	case DeviceDeactivateHashVersionCurrent:
+		break


Do we want to ensure there is no LegacyGroupID specified here?

I guess it does no harm

data/types/bolus/bolus.go

darinkrauss

Still need to make some changes to the versions. Let's chat tomorrow.

darinkrauss · 2024-11-13T02:28:53Z

data/deduplicator/deduplicator/base.go

@@ -24,8 +24,6 @@ func NewBase(name string, version string) (*Base, error) {
 	}
 	if version == "" {
 		return nil, errors.New("version is missing")
-	} else if !net.IsValidSemanticVersion(version) {


Why removed?

darinkrauss · 2024-11-13T02:51:08Z

data/deduplicator/deduplicator/device_deactivate_hash.go

-	DeviceDeactivateHashVersionUnkown  DeviceDeactivateHashVersion = ""
-	DeviceDeactivateHashVersionCurrent DeviceDeactivateHashVersion = "1.1.0"
-	DeviceDeactivateHashVersionLegacy  DeviceDeactivateHashVersion = "0.0.0"
+	DeviceDeactivateHashVersionUnknown = "unknown-hash"


Sorry, I think we have a disconnect. I may not have explained myself very well. These version strings need to remain the same.

There need to be two separate version groups.

First, the version of the DeviceDeactivationHash deduplicator. That would be "0.0.0" (for Jellyfish) and "1.1.0" (for current Platform).

Second, the version of the identity fields, that would be, for simplicity, "0" (for legacy) and "1" (for current). This is what is passed into the datum.IdentityFields function.

The hash function and HashOptions, since it is not specific to the DeviceDeactivateHash deduplicator, should use the identity fields version.

Or, for complete separation of concerns, you could add a third version, the hash version, either "0" or "1".

So, then the code maps from deduplicator version to hash version to identity fields version.

The DeviceDeactivateHash code looks at its version, maps its version to a hash version and creates the HashOptions with that version. Then, the hash code looks at its version, maps its version to an identity field versions and calls IdentityFields with that identity field version.

Then, you've completely isolated the DeviceDeactivationHash deduplicator from the hash functionality from the identity fields functionality. In practice, we definitely have to do the first step (deduplicator version to hash version) because there will be another duplicator for continuous data sets that is not based off the device id, but the data set id, and doesn't deactivate, but deletes.

Technically, we don't actually need the hash version and could just use the identity fields version for both hash function and IdentityFields function.

Does that make sense? Let's chat about this tomorrow.

jh-bate added 30 commits February 1, 2024 17:09

fix type

235c05c

BACK 2746

fc353cb

tests moves to migration utils

0e6783e

basic update tests

2d2ecb5

payload test

bef257e

test for payload and annotations

8dcb33a

remove calc wizard from audit

1b52af2

build mongo query form difference

649bcf5

don't build diff on internal properties

c70e95d

import order

7011293

remove payload

9a0548c

check if cap set or not

c27671a

set last update ID for select

c46998b

set id even when item not updated

4f6c7cf

it has still been processed

don't write the updates yet

9115b92

switch to audit or update process

9c87b14

filter out inner payload changes

329af8c

errors per type

4915b2e

type specific validator changes

b41fbdf

fix payload type

f7334c5

parse for specific types

4a0fe93

move percent so it is deleted in update

56816a7

try to remove empty payload

8912a52

remove payload

204ff7e

type specific updates

ace6538

allow updates

203427e

log if auditing

87a97df

remove needless logging

da9bbe9

tests and updates

5941ddc

trying to sort empty payload

fb540b9

Merge branch 'master' into jf_deduplication

e4d88ab

darinkrauss requested changes Oct 15, 2024

View reviewed changes

jh-bate added 9 commits October 21, 2024 13:58

review updates

5d39959

update to use IdentityFields(version string) ([]string, error)

8328218

review updates

f87f048

cleanup

086b4fd

cleanup identityFields creation

7492d22

review updates

1c6731f

Merge branch 'master' into jf_deduplication

f0cc214

Merge branch 'master' into jf_deduplication

0209295

updates from merge

6d84df2

jh-bate requested a review from darinkrauss October 30, 2024 02:05

darinkrauss requested changes Nov 2, 2024

View reviewed changes

jh-bate added 4 commits November 11, 2024 16:22

review updates

adbd24f

Merge branch 'master' into jf_deduplication

77ffcca

update import for consistency

1a090b0

test updates

f715153

jh-bate requested a review from darinkrauss November 12, 2024 04:29

darinkrauss requested changes Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow legacy deduplication #741

Allow legacy deduplication #741

jh-bate commented Jun 18, 2024 •

edited by jira bot

Loading

tidebot commented Oct 8, 2024

tidebot commented Oct 8, 2024

tidebot commented Oct 8, 2024

jh-bate commented Oct 8, 2024

tidebot commented Oct 8, 2024

tidebot commented Oct 8, 2024

tidebot commented Oct 8, 2024

darinkrauss left a comment

darinkrauss Oct 14, 2024

darinkrauss Oct 14, 2024

darinkrauss Oct 15, 2024

jh-bate Oct 30, 2024 •

edited

Loading

jh-bate Nov 11, 2024

darinkrauss Nov 11, 2024

jh-bate commented Oct 30, 2024

tidebot commented Oct 30, 2024

tidebot commented Oct 30, 2024

tidebot commented Oct 30, 2024

darinkrauss left a comment

darinkrauss Nov 2, 2024

darinkrauss Nov 2, 2024

darinkrauss Nov 2, 2024

darinkrauss Nov 2, 2024

darinkrauss Nov 2, 2024

darinkrauss Nov 2, 2024

darinkrauss Nov 2, 2024

jh-bate Nov 11, 2024

darinkrauss left a comment

darinkrauss Nov 13, 2024

darinkrauss Nov 13, 2024

Allow legacy deduplication #741

Are you sure you want to change the base?

Allow legacy deduplication #741

Conversation

jh-bate commented Jun 18, 2024 • edited by jira bot Loading

tidebot commented Oct 8, 2024

tidebot commented Oct 8, 2024

tidebot commented Oct 8, 2024

jh-bate commented Oct 8, 2024

tidebot commented Oct 8, 2024

tidebot commented Oct 8, 2024

tidebot commented Oct 8, 2024

darinkrauss 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

jh-bate Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jh-bate commented Oct 30, 2024

tidebot commented Oct 30, 2024

tidebot commented Oct 30, 2024

tidebot commented Oct 30, 2024

darinkrauss 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

darinkrauss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jh-bate commented Jun 18, 2024 •

edited by jira bot

Loading

jh-bate Oct 30, 2024 •

edited

Loading