feat: Expire unpublished operations #807

DRK3 · 2021-10-12T22:29:33Z

Signed-off-by: Derek Trider Derek.Trider@securekey.com

codecov · 2021-10-12T22:33:46Z

Codecov Report

Merging #807 (474bdd5) into main (789f5f0) will increase coverage by 0.05%.
The diff coverage is 86.81%.

@@            Coverage Diff             @@
##             main     #807      +/-   ##
==========================================
+ Coverage   89.51%   89.56%   +0.05%     
==========================================
  Files         141      143       +2     
  Lines       12338    12502     +164     
==========================================
+ Hits        11044    11198     +154     
- Misses        799      807       +8     
- Partials      495      497       +2

Impacted Files	Coverage Δ
cmd/orb-server/startcmd/start.go	`73.99% <66.66%> (-0.10%)`	⬇️
pkg/store/operation/unpublished/store.go	`92.59% <77.77%> (-3.16%)`	⬇️
pkg/store/expiry/expiry.go	`91.04% <91.04%> (ø)`
...g/protocolversion/versions/v1_0/factory/factory.go	`100.00% <100.00%> (+4.00%)`	⬆️
pkg/document/util/util.go	`90.16% <0.00%> (-9.84%)`	⬇️
pkg/document/updatehandler/decorator/decorator.go	`100.00% <0.00%> (ø)`
cmd/orb-server/startcmd/params.go	`80.54% <0.00%> (+0.03%)`	⬆️
pkg/cas/resolver/resolver.go	`89.77% <0.00%> (+0.36%)`	⬆️
pkg/document/resolvehandler/resolvehandler.go	`95.95% <0.00%> (+2.71%)`	⬆️

Continue to review full report at Codecov.

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

DRK3 · 2021-10-13T16:24:17Z

pkg/protocolversion/versions/v1_0/factory/factory.go

@@ -67,7 +67,7 @@ func (v *Factory) Create(version string, casClient cas.Client, casResolver ctxco
 	var orbTxnProcessorOpts []txnprocessor.Option

 	if sidetreeCfg.UpdateDocumentStoreEnabled {
-		updateDocumentStore, err := unpublished.New(provider)
+		updateDocumentStore, _, err := unpublished.New(provider)


We do this same call in start.go and I start the expiry service there, which is why I ignore the expiry service returned here. This object is the same as the one we make in start.go. If possible, we should probably pass the unpublished operation store object from start.go through to here instead of creating it twice.

I was thinking that there should be a single expiry service and that multiple stores can register with it to expire their data.

I expected the same.

Done! Take a look at the new design and let me know how it looks.

(Also, my note about passing the unpublished operation store object still stands)

sandrask · 2021-10-13T17:13:21Z

scripts/integration.sh

-go test -run all -count=1 -v -cover . -p 1 -timeout=20m -race
+#export CAS_TYPE=ipfs
+#
+#go test -run all -count=1 -v -cover . -p 1 -timeout=20m -race


you disabled ipfs

Also, we should have BDD test (can be done as separate PR)

Oops - I re-enabled IPFS tests. I've added a TODO for the BDD test - I'll do it in another PR

bstasyszyn · 2021-10-13T20:29:06Z

cmd/orb-server/startcmd/start.go

+		// TODO (#808): Allow expiry interval to be configurable.
+		expiryService = expiry.NewService(defaultExpiryInterval)
+
+		updateDocumentStore.RegisterWithExpiryService(expiryService)


I think this can be hidden by adding the expiry service to the list of providers and then unpublishedstore.New can Register.

The problem is that unpublishedstore.New is called in two places - here and in factory.go, and we don't want to register it twice. What I think would be better is if the unpublished store instance from start.go was passed into factory.go, and that way I could safely add the Register call inside of unpublishedstore.New since I will know that it's only called in one place. However, I see that factory is an interface, so it would have to be changed (or I would have to add it to the config.Sidetree object). Are there any issues with doing this? @sandrask

You should ignore (do not add) duplicate registrations - same store can be used in different places.

@sandrask I could do that... but then I'll need to pass the expiry service into the factory, which we shouldn't do since factory really shouldn't need to know anything about the expiry service. I think the cleanest solution is to create the unpublished store object once in start.go and pass it around where needed, if possible.

Sure you can do that.

bstasyszyn · 2021-10-13T20:31:31Z

cmd/orb-server/startcmd/start.go

+		//              a separate server)
+
+		// TODO (#808): Allow expiry interval to be configurable.
+		expiryService = expiry.NewService(defaultExpiryInterval)


The expiry service can be created outside of this IF. Practically speaking, we will have these features enabled in production.

If I create it outside of this if statement, and updateDocumentStoreEnabled is false, then I will have a pointless expiry service with no registered stores that will still start and still use a go routine. I will still need some mechanism to detect whether to start the expiry service or not. I could add a method to the expiry service so that the caller can check whether there are any registered stores so they can decide whether to start it, but I don't think that's simpler than what I already have. Also note that when there are more stores that need the expiry service, this problem will probably go away since I'll need to move it outside of the if statement anyway.

You mentioned, however, that practically we will have the update document store enabled for production? If that's the case, do we need that option at all? If we can remove it, that will also make the issue go away and make the code simpler (factory.go also checks that option). CC @sandrask

If you don't have any registrations don't start the service.

I would have service running anyway in case that some store registers after start-up. Is this possible?

@sandrask I don't think we will have store registers after start-up... at least not now. I was thinking that I would handle that scenario when/if we need that ability.

@DRK3 @bstasyszyn We can discuss if we need update document store enabled as an option. I always envisioned that we will have clients who will want to 'anchor' operation before using it.

I've left the option there per the discussion @sandrask @bstasyszyn. I've left the expiry service within the if for now - in the future when I add other stores to this service, I'll need to revisit this though.

Signed-off-by: Derek Trider <Derek.Trider@securekey.com>

cla-bot bot added the cla-signed label Oct 12, 2021

DRK3 force-pushed the ExpiryServiceUnpublishedOperations branch 6 times, most recently from c06f6e7 to ef7006e Compare October 13, 2021 16:12

DRK3 commented Oct 13, 2021

View reviewed changes

DRK3 force-pushed the ExpiryServiceUnpublishedOperations branch from ef7006e to eef774d Compare October 13, 2021 16:42

DRK3 marked this pull request as ready for review October 13, 2021 16:58

DRK3 requested review from sandrask and bstasyszyn October 13, 2021 17:03

sandrask reviewed Oct 13, 2021

View reviewed changes

DRK3 force-pushed the ExpiryServiceUnpublishedOperations branch 3 times, most recently from dba5ab2 to 683212f Compare October 13, 2021 20:06

bstasyszyn reviewed Oct 13, 2021

View reviewed changes

DRK3 force-pushed the ExpiryServiceUnpublishedOperations branch from 683212f to 0a2545e Compare October 14, 2021 18:03

feat: Expire unpublished operations

474bdd5

Signed-off-by: Derek Trider <Derek.Trider@securekey.com>

DRK3 force-pushed the ExpiryServiceUnpublishedOperations branch from 0a2545e to 474bdd5 Compare October 14, 2021 19:03

bstasyszyn approved these changes Oct 14, 2021

View reviewed changes

sandrask approved these changes Oct 14, 2021

View reviewed changes

DRK3 merged commit ef3a493 into trustbloc:main Oct 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Expire unpublished operations #807

feat: Expire unpublished operations #807

DRK3 commented Oct 12, 2021

codecov bot commented Oct 12, 2021 •

edited

Loading

DRK3 Oct 13, 2021

bstasyszyn Oct 13, 2021

sandrask Oct 13, 2021

DRK3 Oct 13, 2021

sandrask Oct 13, 2021

sandrask Oct 13, 2021

DRK3 Oct 13, 2021

bstasyszyn Oct 13, 2021

DRK3 Oct 13, 2021

sandrask Oct 13, 2021 •

edited

Loading

DRK3 Oct 13, 2021

sandrask Oct 13, 2021

DRK3 Oct 14, 2021

bstasyszyn Oct 13, 2021

DRK3 Oct 13, 2021 •

edited

Loading

sandrask Oct 13, 2021

sandrask Oct 13, 2021

DRK3 Oct 13, 2021

sandrask Oct 13, 2021

DRK3 Oct 14, 2021

feat: Expire unpublished operations #807

feat: Expire unpublished operations #807

Conversation

DRK3 commented Oct 12, 2021

codecov bot commented Oct 12, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandrask Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DRK3 Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 12, 2021 •

edited

Loading

sandrask Oct 13, 2021 •

edited

Loading

DRK3 Oct 13, 2021 •

edited

Loading