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

Critical objectstorage fix, Storable.ImportStorable, ShrinkingMap.Set return value and more #440

Merged
merged 11 commits into from
Jan 10, 2023

Conversation

hmoog
Copy link
Contributor

@hmoog hmoog commented Dec 7, 2022

Description of change

  • ⚠️ Fix objectstorage bug: whenever an object is "ComputedIfAbsent", it is not marked as modified, leading to the batchwriter to not persisting the object in the storage.
  • Make the Storable model initializeable from another model.
  • ShrinkingMap.Set was not correctly returning the updated status of an object; the value should be constrained to comparable. Return if the entry was instead created, regarded of its value update.
  • The TypedStore should Get and Set base values instead of pointers to such values. This aids the storage of basic types.
  • Deprecate WithRealm in favor of WithExtendedRealm to avoid potential prefix clashes.

@karimodm karimodm changed the title Provide Storable.ImportStorable Storable.ImportStorable, ShrinkingMap.Set return value and flatten TypedStore Dec 12, 2022
@karimodm karimodm changed the title Storable.ImportStorable, ShrinkingMap.Set return value and flatten TypedStore Critical objectstorage fix, Storable.ImportStorable, ShrinkingMap.Set return value and more Dec 13, 2022
@@ -68,12 +68,12 @@ func New[K comparable, V any](opts ...Option) *ShrinkingMap[K, V] {
return shrinkingMap
}

// Set adds a key-value pair to the map. It returns true if the key exists.
func (s *ShrinkingMap[K, V]) Set(key K, value V) (updated bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changed the logic completely 😱
We use the shrining map also in other projects....

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I checked all projects I know. We don't use the return value by now.

@karimodm karimodm merged commit 1b78782 into master Jan 10, 2023
@karimodm karimodm deleted the feat/storable-import branch January 10, 2023 10:24
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.

3 participants