Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

History type support #176

Merged
merged 4 commits into from
Oct 12, 2023
Merged

Conversation

DJAndries
Copy link
Collaborator

@DJAndries DJAndries commented Oct 6, 2023

Adds support for the new Chromium history type.

History sync entities will use the ClientTagHash as a primary key, instead of the IdString. Differing version numbers between old and new items will be ignored when updating.

- Use client_tag_hash as ID for history type
- No tag items needed for history type
- No version checking for history type when update records
@DJAndries DJAndries force-pushed the history-type-support branch from f1cf8aa to d4e8a43 Compare October 6, 2023 06:20
@@ -12,7 +12,7 @@
],
"GlobalSecondaryIndexes": [
{
"IndexName": "ClientIDDataTypeMtimeIndex",
"IndexName": "ClientIDDataTypeMtimeIndexV2",
Copy link
Collaborator Author

@DJAndries DJAndries Oct 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a db change is needed for this to work:

  • enabling ttl for the ExpirationTime attribute

@DJAndries DJAndries force-pushed the history-type-support branch from f9c2404 to a1ec741 Compare October 7, 2023 07:18
Copy link
Member

@yrliou yrliou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, just some minor comments for readability.
Also please make sure to have a green CI test.

command/command.go Outdated Show resolved Hide resolved
command/command.go Outdated Show resolved Hide resolved
datastore/sync_entity.go Show resolved Hide resolved
datastore/sync_entity.go Outdated Show resolved Hide resolved
datastore/sync_entity.go Outdated Show resolved Hide resolved
datastore/sync_entity.go Show resolved Hide resolved
Comment on lines +251 to +255
log.Error().Err(err).Msg("Insert history sync entity failed")
rspType := sync_pb.CommitResponse_TRANSIENT_ERROR
entryRsp.ResponseType = &rspType
entryRsp.ErrorMessage = aws.String(fmt.Sprintf("Insert history sync entity failed: %v", err.Error()))
continue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Maybe Error checking if client_unique_tag exists rather than Insert history sync entity failed is more clear?

@DJAndries DJAndries merged commit a0d6380 into brave:master Oct 12, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants