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

Honor current TTL in DMap.Incr and DMap.Decr methods #172

Merged
merged 6 commits into from
Aug 4, 2022
Merged

Honor current TTL in DMap.Incr and DMap.Decr methods #172

merged 6 commits into from
Aug 4, 2022

Conversation

hasit
Copy link
Contributor

@hasit hasit commented Jul 7, 2022

Since Incr and Decr operations internally do a Get followed by Put, the TTL value continuously . This change brings the following 2 changes to honor the TTL:

  1. Update DMap.loadCurrentAtomicInt to return current TTL value from entry
  2. Update DMAP.atomicIncrDecr to received the current TTL after calling DMap.loadCurrentAtomicInt and then update e.putConfig before putting the entry. This update signifies that the put contains an expiry and passes the new expiry value.

Since Incr and Decr operations internally do a Get followed by Put, the TTL value continuously . This change brings the following 2 changes to honor the TTL:
1. Update DMap.loadCurrentAtomicInt to return current TTL value from entry
2. Update DMAP.atomicIncrDecr to received the current TTL after calling DMap.loadCurrentAtomicInt and then update e.putConfig before putting the entry. This update signifies that the put contains an expiry and passes the new expiry value.
@hasit hasit closed this Jul 7, 2022
Honor current TTL in DMap.Incr and DMap.Decr methods
@hasit hasit deleted the incr-ttl branch July 7, 2022 19:14
@buraksezer
Copy link
Owner

Hello @hasit, thank you for your contribution. Your changes look good to me. Why did you close this PR?

@hasit hasit restored the incr-ttl branch July 7, 2022 19:41
@hasit hasit reopened this Jul 7, 2022
@hasit
Copy link
Contributor Author

hasit commented Jul 7, 2022

@buraksezer Hey, I closed it because I thought it'd be more appropriate to discuss this change over an issue before I opened a PR. However, if this looks good to you, we can merge it.

I am also looking to get this fix into v0.4.x. If that is desirable, I can open another PR to patch it.

internal/dmap/atomic.go Outdated Show resolved Hide resolved
internal/dmap/atomic.go Outdated Show resolved Hide resolved
@buraksezer
Copy link
Owner

@buraksezer Hey, I closed it because I thought it'd be more appropriate to discuss this change over an issue before I opened a PR. However, if this looks good to you, we can merge it.

Thank you for your contribution @hasit. We can merge it after using PX instead of EX. Could you please add unit tests for this feature?

I am also looking to get this fix into v0.4.x. If that is desirable, I can open another PR to patch it.

v0.4.x will go into maintenance mode after releasing the next version. I think it's good to keep the API and its behavior unchanged.

@buraksezer
Copy link
Owner

I saw the release for v0.4.5. It's already done. So you can send the PR. @hasit

@hasit
Copy link
Contributor Author

hasit commented Jul 13, 2022

@buraksezer any suggestions on how to test this?

@buraksezer
Copy link
Owner

Obviously, this isn't a unit test, but you can try a similar approach.

func TestDMap_Atomic_Decr(t *testing.T) {
	cluster := testcluster.New(NewService)
	s := cluster.AddMember(nil).(*Service)
	defer cluster.Shutdown()

	var wg sync.WaitGroup
	var start chan struct{}
	key := "decr"

	ctx := context.Background()

	decr := func(dm *DMap) {
		<-start
		defer wg.Done()

		_, err := dm.Decr(ctx, key, 1)
		if err != nil {
			s.log.V(2).Printf("[ERROR] Failed to call Decr: %v", err)
			return
		}
	}

	dm, err := s.NewDMap("atomic_test")
	require.NoError(t, err)

	start = make(chan struct{})
	for i := 0; i < 100; i++ {
		wg.Add(1)
		go decr(dm)
	}
	close(start)
	wg.Wait()

	res, err := dm.Get(context.Background(), key)
	require.NoError(t, err)

	var value int
	err = resp.Scan(res.Value(), &value)
	require.NoError(t, err)
	require.Equal(t, -100, value)
}

In this test, Olric tries to run an atomic decrement on a key. Concurrent goroutines try to decrease a number by 1. Then, it checks the value with Get command. Basically, we can use the same method to test your new feature. dm.Get returns a storage.Entry instance and it as TTL method.

See the following:

gr, err := dm.Get(context.Background(), key)
require.NoError(t, err)
fmt.Println(gr.TTL())

hasit added 2 commits August 3, 2022 11:28
- Pass `context.Background()` to newEnv in `readRepair` intead of `nil` for the sake of being explicit
- Update some test variable creations to use `:=` instead of `var`
@hasit
Copy link
Contributor Author

hasit commented Aug 3, 2022

@buraksezer 4e3c2a7 adds a basic test for checking if the returned TTL is within a certain duration. Let me know if more tests are required for this.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 3, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@buraksezer buraksezer merged commit 33ac0e0 into buraksezer:master Aug 4, 2022
@buraksezer
Copy link
Owner

Thank you for your contribution, @hasit!

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