Feature: Add setOrGet Function to Cache Implementation

[ndjuric-bit]

Overview:

This pull request introduces a new feature to our cache implementation by adding the SetOrGet function. This function allows clients to efficiently set a cache entry if it doesn't exist or retrieve the existing value associated with a given key. It returns the actual value, a boolean indicating whether the entry was loaded from the cache or not, and any potential error.

Function Signature::

SetOrGet(key string, entry []byte) (actual []byte, loaded bool, err error)

Parameters:

• key: A string representing the cache key to look up.
• entry: A byte slice containing the value to set in the cache if the key doesn't exist.

Return Values:

• actual: The actual value associated with the key in the cache, which may be the newly set value or the existing cached value.
• loaded: A boolean indicating whether the entry was loaded from the cache (true) or if it was set with the provided entry (false).
• err: An error that is returned in case of any issues during the set or get operation.

Example:

actualValue, loadedFromCache, err := cache.SetOrGet("my_key", []byte("new_value"))
if err != nil {
    // Handle the error.
} else {
    if loadedFromCache {
        // The value was loaded from the cache.
        fmt.Printf("Value for key 'my_key' found in the cache: %s\n", actualValue)
    } else {
        // The value was set in the cache.
        fmt.Printf("Value for key 'my_key' was set to: %s\n", actualValue)
    }
}

Testing:

This pull request includes unit tests to ensure the correct behavior of the SetOrGet function. The tests cover various scenarios, including cache hits and cache misses.

Impact on Existing Code:

This change introduces a new function to cache system. Existing code that uses the cache may benefit from this function for efficient get-or-set operations, but it should not negatively impact the existing functionality. This change is designed to be backward compatible.

Documentation:

The function's usage has been documented, and code comments have been updated to reflect the introduction of the SetOrGet function.

Reviewer Instructions:

Please review the code changes and documentation to ensure that the SetOrGet function is correctly implemented and well-documented. Verify that the function behaves as expected, handles errors gracefully, and is a useful addition to our cache system.
In case of hash collision behaviour of get function is held plus old data is deleted and new one is stored

Testing Instructions:
Verify that the unit tests pass and consider adding additional test cases as needed to cover any edge cases.

Changelog:

• Added SetOrGet function to the cache module.
  

• Updated documentation and comments to include usage examples.
  

[codecov-commenter]

Codecov Report

Merging #383 (dd4212d) into main (58fe2d2) will increase coverage by 0.27%.
The diff coverage is 82.75%.

❗ Current head dd4212d differs from pull request most recent head 4c79098. Consider uploading reports for the commit 4c79098 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main     #383      +/-   ##
==========================================
+ Coverage   88.66%   88.94%   +0.27%     
==========================================
  Files          15       15              
  Lines         794      823      +29     
==========================================
+ Hits          704      732      +28     
  Misses         76       76              
- Partials       14       15       +1     

Files	Coverage Δ
bigcache.go	93.18% <100.00%> (+0.21%)	⬆️
shard.go	90.97% <80.00%> (+0.47%)	⬆️

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58fe2d2...4c79098. Read the comment docs.

[janisz]

I think this is problematic. We have a read lock and then we lock to insert something. Another goroutine might hijack our lock in meantime.

[ndjuric-bit]

Thanks for answer
i think i found somewhere that this actions(locks) are atomic and in that case will not be possible to jump between them and with performance in mind i have sent code you have seen. Original code is below it is more readable and if you sad more robust, than i will change request with code below.

Original code was

func (s *cacheShard) setOrGet(key string, hashedKey uint64, entry []byte) (actual []byte, loaded bool, err error) {
	s.lock.Lock()
	defer s.lock.Unlock()

	wrappedEntry, err := s.getWrappedEntry(hashedKey)
	if err == nil {
		if entryKey := readKeyFromEntry(wrappedEntry); key == entryKey {
			actual = readEntry(wrappedEntry)
			s.hit(hashedKey)
			return actual, true, nil
		} else {

			s.collision()
			if s.isVerbose {
				s.logger.Printf("Collision detected. Both %q and %q have the same hash %x", key, entryKey, hashedKey)
			}

			delete(s.hashmap, hashedKey)
			s.onRemove(wrappedEntry, Deleted)
			if s.statsEnabled {
				delete(s.hashmapStats, hashedKey)
			}
			resetHashFromEntry(wrappedEntry)
		}
	} else if !errors.Is(err, ErrEntryNotFound) {
		return entry, false, err
	}

	return entry, false, s.addNewWithoutLock(key, hashedKey, entry)
}

[janisz]

@ndjuric-bit Thanks for this. I think we need to solve issue with concurrent updates during setOrGet with current implementation it's not locked so there might an override. I think this would be a rare case but possible so it needs to be documented. I also miss some tests how it behaves when error happens.

[ndjuric-bit]

@janisz Is there something needed from my side more ?