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

Only store unique store and forward messages #1862

Merged
merged 1 commit into from
May 20, 2020

Conversation

neonknight64
Copy link
Contributor

Description

  • These changes allow only unique messages to be stored by the store and forward system.
  • This is achieved by comparing the body hash of a new message to the hashes of the currently stored set of messages. Only if the body content is unique is the message stored by the store and forward database.

Motivation and Context

Limit unnecessary storage of messages with duplicate content.

How Has This Been Tested?

Modified the store and forward database insert_message test to also test the behaviour when a duplicate message is inserted.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Feature refactor (No new feature or functional changes, but performance or technical debt improvements)
  • New Tests
  • Documentation

Checklist:

  • I'm merging against the development branch.
  • I ran cargo-fmt --all before pushing.
  • I have squashed my commits into a single commit.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@neonknight64 neonknight64 force-pushed the yuko-saf_duplicate_message_insert branch from 06bf879 to 3896e6e Compare May 14, 2020 13:49
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Awesome! Just a suggestion to use the database engine to do unique checking.

comms/dht/src/store_forward/database/mod.rs Outdated Show resolved Hide resolved
@neonknight64 neonknight64 force-pushed the yuko-saf_duplicate_message_insert branch from 3896e6e to 8cf9b7c Compare May 18, 2020 06:49
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Thanks - tested on my base node and this works - just need to complete the down migration

ALTER TABLE stored_messages
ADD body_hash TEXT;

CREATE UNIQUE INDEX uidx_stored_messages_body_hash ON stored_messages (body_hash);
Copy link
Member

Choose a reason for hiding this comment

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

Tested migration and works well (nulls are not counted towards the unique index as expected)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is great!

- These changes allow only unique messages to be stored by the store and forward system.
- This is achieved by comparing the body hash of a new message to the hashes of the currently stored set of messages. Only if the body content is unique is the message stored by the store and forward database.
- Modified the store and forward database insert_message test to also test the behaviour when a duplicate message is inserted.
@neonknight64 neonknight64 force-pushed the yuko-saf_duplicate_message_insert branch from 8cf9b7c to 3c96423 Compare May 18, 2020 09:15
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

👍

@sdbondi sdbondi merged commit fd3bebb into development May 20, 2020
@sdbondi sdbondi deleted the yuko-saf_duplicate_message_insert branch May 22, 2020 11:46
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