docstore: distinguish ordered from unordered action lists #1742

jba · 2019-04-02T00:04:29Z

Clarify the meaning of running an ActionList in order.
Add the ActionList.Unordered method, so users can request out-of-order execution.
This also affects errors: in Unordered mode, all actions are run regardless of errors.
To accommodate unordered-mode errors, change ActionList.Do to return a special error
type that can represent multiple errors along with the corresponding actions' positions.

Implementing Unordered is left for a subsequent PR.

Updates #1660.

- Clarify the meaning of running an ActionList in order. - Add the ActionList.Unordered method, so users can request out-of-order execution. This also affects errors: in Unordered mode, all actions are run regardless of errors. - To accommodate unordered-mode errors, change ActionList.Do to return a special error type that can represent multiple errors along with the corresponding actions' positions. Implementing Unordered is left for a subsequent PR. Updates google#1660.

codecov · 2019-04-02T00:24:01Z

Codecov Report

Merging #1742 into master will decrease coverage by 0.19%.
The diff coverage is 58.33%.

@@            Coverage Diff            @@
##           master    #1742     +/-   ##
=========================================
- Coverage   73.89%   73.69%   -0.2%     
=========================================
  Files          75       75             
  Lines        7764     7801     +37     
=========================================
+ Hits         5737     5749     +12     
- Misses       1623     1642     +19     
- Partials      404      410      +6

Impacted Files	Coverage Δ
internal/docstore/mongodocstore/mongo.go	`0% <0%> (ø)`	⬆️
internal/docstore/dynamodocstore/dynamo.go	`69.41% <50%> (-0.83%)`	⬇️
internal/docstore/memdocstore/mem.go	`76.47% <55.55%> (-1.31%)`	⬇️
internal/docstore/firedocstore/fs.go	`66.14% <60%> (-0.52%)`	⬇️
internal/docstore/docstore.go	`73.8% <65.95%> (-9.7%)`	⬇️
pubsub/mempubsub/mem.go	`85.29% <0%> (-1.48%)`	⬇️
internal/retry/retry.go	`100% <0%> (+11.76%)`	⬆️

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 21a88a2...253db9a. Read the comment docs.

shantuo · 2019-04-02T14:18:17Z

internal/docstore/driver/driver.go

+
+// An ActionListError contains all the errors encountered from a call to RunActions,
+// and the positions of the corresponding actions.
+type ActionListError []struct {


Maybe ActionListErrors or ActionErrorList?

I think singular (ActionListError) makes a little more sense. It is one error, even though it holds multiple things. For comparison, Hashicorp's version of this is called multierror.

internal/docstore/docstore.go

jba · 2019-04-02T17:27:14Z

PTAL.

internal/docstore/docstore.go

vangent · 2019-04-03T16:42:01Z

internal/docstore/docstore.go

+// Do executes the action list.
+// In ordered mode (when the Unordered method was not called on the list), execution
+// will stop after the first action that fails.
+// In unordered mode, all the actions will be executed. In either mode, as a special


Consider moving this "none of the actions" thing to be first? E.g.,

// Do executes the action list. // // If any of the actions are invalid (for example, ...), it returns an error, and no actions are executed. // // If ordered mode... // // In unordered mode...

I still think this (the "special case") should be moved up above, or at least not be part of the "unordered mode" paragraph (since it applies to both modes).

internal/docstore/docstore.go

vangent · 2019-04-03T16:44:28Z

internal/docstore/docstore.go

+// An ActionListError is returned by ActionList.Do. It contains all the errors
+// encountered while executing the ActionList, and the positions of the corresponding
+// actions.
+type ActionListError = driver.ActionListError


I think you should copy the type here; otherwise, the end user has to look at the driver package to see what an ActionListError consists of.

internal/docstore/driver/driver.go

vangent · 2019-04-03T16:47:58Z

internal/docstore/driver/driver.go

+	return strings.Join(s, "; ")
+}
+
+func (e ActionListError) Unwrap() error {


I don't understand what "Unwrap" is for. I find it surprising that it returns nil if there's more than one error. What is this used for?

OK, now I see it used above. Maybe "SingleError" or something? At least a docstring.

This is for conformance with the new errors stuff. See https://tip.golang.org/pkg/errors/#Wrapper.

vangent · 2019-04-03T16:49:06Z

internal/docstore/docstore.go

+		return alerr
+	}
+	alerr = l.coll.driver.RunActions(ctx, das, l.unordered)
+	if alerr != nil { // Explicit non-nil check is necessary because alerr is not of type error.


Why is the nil check needed? Isn't it always safe to range over the slice?

It is safe to range over a nil slice. But I need the explicit nil check somewhere. If there is no error, I want to return the interface value (nil, nil) from this function, not (nil, ActionListError). See https://golang.org/doc/faq#nil_error.

My comment on this line was supposed to explain this. Let me know how I can improve it.

Oh I see. I was misled by the comment being right before the range.

Maybe:

alerr = ... if len(alerr) == 0 { return nil // Explicitly return nil, because alerr is not of type error } for i := range ...{} return alerr ``` I don't feel that strongly about this, but I didn't get the comment on first read.

internal/docstore/driver/driver.go

internal/docstore/drivertest/drivertest.go

jba · 2019-04-03T18:04:52Z

PTAL.

vangent · 2019-04-03T18:26:48Z

internal/docstore/docstore.go

+// Do executes the action list.
+// In ordered mode (when the Unordered method was not called on the list), execution
+// will stop after the first action that fails.
+// In unordered mode, all the actions will be executed. In either mode, as a special


I still think this (the "special case") should be moved up above, or at least not be part of the "unordered mode" paragraph (since it applies to both modes).

vangent · 2019-04-03T18:28:02Z

internal/docstore/docstore.go

+// In unordered mode, all the actions will be executed. In either mode, as a special
+// case, none of the actions will be executed if any is invalid (for example, a Put
+// whose document is missing its key field).
+func (l *ActionList) Do(ctx context.Context) error {


If this function always returns ActionListError, maybe state that more explicitly?

vangent · 2019-04-03T18:30:32Z

internal/docstore/docstore.go

+		return alerr
+	}
+	alerr = l.coll.driver.RunActions(ctx, das, l.unordered)
+	if alerr != nil { // Explicit non-nil check is necessary because alerr is not of type error.


Oh I see. I was misled by the comment being right before the range.

Maybe:

alerr = ... if len(alerr) == 0 { return nil // Explicitly return nil, because alerr is not of type error } for i := range ...{} return alerr ``` I don't feel that strongly about this, but I didn't get the comment on first read.

…tore-actionlist-ordering

jba requested review from shantuo and vangent April 2, 2019 00:04

go-cloud-bot bot assigned shantuo and vangent Apr 2, 2019

googlebot added the cla: yes Google CLA has been signed! label Apr 2, 2019

shantuo requested changes Apr 2, 2019

View reviewed changes

.

1fe24f7

shantuo approved these changes Apr 2, 2019

View reviewed changes

vangent reviewed Apr 3, 2019

View reviewed changes

jba added 2 commits April 3, 2019 13:49

.

dda52d2

.

e7ed222

vangent approved these changes Apr 3, 2019

View reviewed changes

jba added 2 commits April 3, 2019 16:29

Merge branch 'master' of https://github.com/google/go-cloud into docs…

4603888

…tore-actionlist-ordering

.

253db9a

jba merged commit b723f63 into google:master Apr 3, 2019

jba deleted the docstore-actionlist-ordering branch April 3, 2019 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docstore: distinguish ordered from unordered action lists #1742

docstore: distinguish ordered from unordered action lists #1742

jba commented Apr 2, 2019

codecov bot commented Apr 2, 2019 •

edited

shantuo Apr 2, 2019

jba Apr 2, 2019

jba commented Apr 2, 2019

vangent Apr 3, 2019

vangent Apr 3, 2019

jba Apr 3, 2019

vangent Apr 3, 2019

jba Apr 3, 2019

vangent Apr 3, 2019

vangent Apr 3, 2019

jba Apr 3, 2019

vangent Apr 3, 2019

jba Apr 3, 2019

vangent Apr 3, 2019

jba commented Apr 3, 2019

vangent Apr 3, 2019

vangent Apr 3, 2019

jba Apr 3, 2019

vangent Apr 3, 2019

docstore: distinguish ordered from unordered action lists #1742

docstore: distinguish ordered from unordered action lists #1742

Conversation

jba commented Apr 2, 2019

codecov bot commented Apr 2, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jba commented Apr 2, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jba commented Apr 3, 2019

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 Apr 2, 2019 •

edited