config: support zrepl's day and week units for snapshotting.interval

Originally, I had a patch that would replace all usages of
time.Duration in package config with the new config.Duration
types, but:
1. these are all timeouts/retry intervals that have default values.
   Most users don't touch them, and if they do, they don't need
   day or week units.
2. go-yaml's error reporting for yaml.Unmarshaler is inferior to
   built-in types (line numbers are missing, so the error would not have
   sufficient context)

fixes https://github.com/zrepl/zrepl/issues/486

.circleci/config.yml

@@ -150,7 +150,7 @@ parameters:
 
   release_docker_baseimage_tag:
     type: string
-    default: "1.19"
+    default: "1.17"
 
 workflows:
   version: 2
@@ -160,15 +160,15 @@ workflows:
     jobs:
       - quickcheck-docs
       - quickcheck-go: &quickcheck-go-smoketest
-          name: quickcheck-go-amd64-linux-1.19
+          name: quickcheck-go-amd64-linux-1.17
-          goversion: &latest-go-release "1.19"
+          goversion: &latest-go-release "1.17"
           goos: linux
           goarch: amd64
       - test-go-on-latest-go-release:
           goversion: *latest-go-release
       - quickcheck-go:
           requires:
-            - quickcheck-go-amd64-linux-1.19 #quickcheck-go-smoketest.name
+            - quickcheck-go-amd64-linux-1.17 #quickcheck-go-smoketest.name
           matrix: &quickcheck-go-matrix
             alias: quickcheck-go-matrix
             parameters:
@@ -249,7 +249,7 @@ jobs:
       goarch:
         type: string
     docker:
-      - image: cimg/go:<<parameters.goversion>>
+      - image: circleci/golang:<<parameters.goversion>>
     environment:
       GOOS: <<parameters.goos>>
       GOARCH: <<parameters.goarch>>
@@ -286,7 +286,7 @@ jobs:
       goversion:
         type: string
     docker:
-      - image: cimg/go:<<parameters.goversion>>
+      - image: circleci/golang:<<parameters.goversion>>
     steps:
       - checkout
       - restore-cache-gomod

Makefile

@@ -28,7 +28,7 @@ GO_BUILDFLAGS := $(GO_MOD_READONLY) $(GO_EXTRA_BUILDFLAGS)
 GO_BUILD := $(GO_ENV_VARS) $(GO) build $(GO_BUILDFLAGS) -ldflags $(GO_LDFLAGS)
 GOLANGCI_LINT := golangci-lint
 GOCOVMERGE := gocovmerge
-RELEASE_DOCKER_BASEIMAGE_TAG ?= 1.19
+RELEASE_DOCKER_BASEIMAGE_TAG ?= 1.17
 RELEASE_DOCKER_BASEIMAGE ?= golang:$(RELEASE_DOCKER_BASEIMAGE_TAG)
 
 ifneq ($(GOARM),)

build/go.mod

@@ -4,10 +4,13 @@ go 1.12
 
 require (
 	github.com/alvaroloes/enumer v1.1.1
-	github.com/golangci/golangci-lint v1.50.1
+	github.com/golangci/golangci-lint v1.35.2
+	github.com/golangci/misspell v0.3.4 // indirect
+	github.com/golangci/revgrep v0.0.0-20180812185044-276a5c0a1039 // indirect
+	github.com/spf13/afero v1.2.2 // indirect
 	github.com/spf13/jwalterweatherman v1.1.0 // indirect
 	github.com/wadey/gocovmerge v0.0.0-20160331181800-b5bfa59ec0ad
-	golang.org/x/tools v0.2.0
+	golang.org/x/tools v0.0.0-20210105210202-9ed45478a130
 	google.golang.org/grpc/cmd/protoc-gen-go-grpc v1.1.0 // indirect
-	google.golang.org/protobuf v1.28.0
+	google.golang.org/protobuf v1.25.0
 )

client/zfsabstractions.go

@@ -57,7 +57,7 @@ func (f *zabsFilterFlags) registerZabsFilterFlags(s *pflag.FlagSet, verb string)
 	for v := range endpoint.AbstractionTypesAll {
 		variants = append(variants, string(v))
 	}
-	sort.Strings(variants)
+	variants = sort.StringSlice(variants)
 	variantsJoined := strings.Join(variants, "|")
 	s.Var(&f.Types, "type", fmt.Sprintf("only %s holds of the specified type [default: all] [comma-separated list of %s]", verb, variantsJoined))
 

config/config.go

@@ -6,8 +6,6 @@ import (
 	"log/syslog"
 	"os"
 	"reflect"
-	"regexp"
-	"strconv"
 	"time"
 
 	"github.com/pkg/errors"
@@ -190,13 +188,10 @@ func (i *PositiveDurationOrManual) UnmarshalYAML(u func(interface{}, bool) error
 		return fmt.Errorf("value must not be empty")
 	default:
 		i.Manual = false
-		i.Interval, err = time.ParseDuration(s)
+		i.Interval, err = parsePositiveDuration(s)
 		if err != nil {
 			return err
 		}
-		if i.Interval <= 0 {
-			return fmt.Errorf("value must be a positive duration, got %q", s)
-		}
 	}
 	return nil
 }
@@ -230,7 +225,7 @@ type SnapshottingEnum struct {
 type SnapshottingPeriodic struct {
 	Type     string            `yaml:"type"`
 	Prefix   string            `yaml:"prefix"`
-	Interval time.Duration `yaml:"interval,positive"`
+	Interval *PositiveDuration `yaml:"interval"`
 	Hooks    HookList          `yaml:"hooks,optional"`
 }
 
@@ -715,41 +710,3 @@ func ParseConfigBytes(bytes []byte) (*Config, error) {
 	}
 	return c, nil
 }
-
-var durationStringRegex *regexp.Regexp = regexp.MustCompile(`^\s*(\d+)\s*(s|m|h|d|w)\s*$`)
-
-func parsePositiveDuration(e string) (d time.Duration, err error) {
-	comps := durationStringRegex.FindStringSubmatch(e)
-	if len(comps) != 3 {
-		err = fmt.Errorf("does not match regex: %s %#v", e, comps)
-		return
-	}
-
-	durationFactor, err := strconv.ParseInt(comps[1], 10, 64)
-	if err != nil {
-		return 0, err
-	}
-	if durationFactor <= 0 {
-		return 0, errors.New("duration must be positive integer")
-	}
-
-	var durationUnit time.Duration
-	switch comps[2] {
-	case "s":
-		durationUnit = time.Second
-	case "m":
-		durationUnit = time.Minute
-	case "h":
-		durationUnit = time.Hour
-	case "d":
-		durationUnit = 24 * time.Hour
-	case "w":
-		durationUnit = 24 * 7 * time.Hour
-	default:
-		err = fmt.Errorf("contains unknown time unit '%s'", comps[2])
-		return
-	}
-
-	d = time.Duration(durationFactor) * durationUnit
-	return
-}

config/config_duration.go

@@ -0,0 +1,106 @@
+package config
+
+import (
+	"errors"
+	"fmt"
+	"regexp"
+	"strconv"
+	"time"
+
+	"github.com/kr/pretty"
+	"github.com/zrepl/yaml-config"
+)
+
+type Duration struct{ d time.Duration }
+
+func (d Duration) Duration() time.Duration { return d.d }
+
+var _ yaml.Unmarshaler = &Duration{}
+
+func (d *Duration) UnmarshalYAML(unmarshal func(v interface{}, not_strict bool) error) error {
+	var s string
+	err := unmarshal(&s, false)
+	if err != nil {
+		return err
+	}
+	d.d, err = parseDuration(s)
+	if err != nil {
+		d.d = 0
+		return &yaml.TypeError{Errors: []string{fmt.Sprintf("cannot parse value %q: %s", s, err)}}
+	}
+	return nil
+}
+
+type PositiveDuration struct{ d Duration }
+
+var _ yaml.Unmarshaler = &PositiveDuration{}
+
+func (d PositiveDuration) Duration() time.Duration { return d.d.Duration() }
+
+func (d *PositiveDuration) UnmarshalYAML(unmarshal func(v interface{}, not_strict bool) error) error {
+	err := d.d.UnmarshalYAML(unmarshal)
+	if err != nil {
+		return err
+	}
+	if d.d.Duration() <= 0 {
+		return fmt.Errorf("duration must be positive, got %s", d.d.Duration())
+	}
+	return nil
+}
+
+func parsePositiveDuration(e string) (time.Duration, error) {
+	d, err := parseDuration(e)
+	if err != nil {
+		return d, err
+	}
+	if d <= 0 {
+		return 0, errors.New("duration must be positive integer")
+	}
+	return d, err
+}
+
+var durationStringRegex *regexp.Regexp = regexp.MustCompile(`^\s*([\+-]?\d+)\s*(|s|m|h|d|w)\s*$`)
+
+func parseDuration(e string) (d time.Duration, err error) {
+	comps := durationStringRegex.FindStringSubmatch(e)
+	if comps == nil {
+		err = fmt.Errorf("must match %s", durationStringRegex)
+		return
+	}
+	if len(comps) != 3 {
+		panic(pretty.Sprint(comps))
+	}
+
+	durationFactor, err := strconv.ParseInt(comps[1], 10, 64)
+	if err != nil {
+		return 0, err
+	}
+
+	var durationUnit time.Duration
+	switch comps[2] {
+	case "":
+		if durationFactor != 0 {
+			err = fmt.Errorf("missing time unit")
+			return
+		} else {
+			// It's the case where user specified '0'.
+			// We want to allow this, just like time.ParseDuration.
+		}
+	case "s":
+		durationUnit = time.Second
+	case "m":
+		durationUnit = time.Minute
+	case "h":
+		durationUnit = time.Hour
+	case "d":
+		durationUnit = 24 * time.Hour
+	case "w":
+		durationUnit = 24 * 7 * time.Hour
+	default:
+		err = fmt.Errorf("contains unknown time unit '%s'", comps[2])
+		return
+	}
+
+	d = time.Duration(durationFactor) * durationUnit
+	return
+}

config/config_snapshotting_test.go

@@ -38,6 +38,13 @@ jobs:
     interval: 10m
 `
 
+	periodicDaily := `
+  snapshotting:
+    type: periodic
+    prefix: zrepl_
+    interval: 1d
+`
+
 	hooks := `
   snapshotting:
     type: periodic
@@ -74,7 +81,15 @@ jobs:
 		c = testValidConfig(t, fillSnapshotting(periodic))
 		snp := c.Jobs[0].Ret.(*PushJob).Snapshotting.Ret.(*SnapshottingPeriodic)
 		assert.Equal(t, "periodic", snp.Type)
-		assert.Equal(t, 10*time.Minute, snp.Interval)
+		assert.Equal(t, 10*time.Minute, snp.Interval.Duration())
+		assert.Equal(t, "zrepl_", snp.Prefix)
+	})
+
+	t.Run("periodicDaily", func(t *testing.T) {
+		c = testValidConfig(t, fillSnapshotting(periodicDaily))
+		snp := c.Jobs[0].Ret.(*PushJob).Snapshotting.Ret.(*SnapshottingPeriodic)
+		assert.Equal(t, "periodic", snp.Type)
+		assert.Equal(t, 24*time.Hour, snp.Interval.Duration())
 		assert.Equal(t, "zrepl_", snp.Prefix)
 	})
 

config/config_test.go

@@ -44,8 +44,7 @@ func TestSampleConfigsAreParsedWithoutErrors(t *testing.T) {
 }
 
 // template must be a template/text template with a single '{{ . }}' as placeholder for val
-//
+//nolint[:deadcode,unused]
-//nolint:deadcode,unused
 func testValidConfigTemplate(t *testing.T, tmpl string, val string) *Config {
 	tmp, err := template.New("master").Parse(tmpl)
 	if err != nil {

daemon/hooks/hook_docs.go

@@ -5,7 +5,7 @@
 //
 // This package also provides all supported hook type implementations and abstractions around them.
 //
-// # Use For Other Kinds Of ExpectStepReports
+// Use For Other Kinds Of ExpectStepReports
 //
 // This package REQUIRES REFACTORING before it can be used for other activities than snapshots, e.g. pre- and post-replication:
 //
@@ -15,7 +15,7 @@
 // The hook implementations should move out of this package.
 // However, there is a lot of tight coupling which to untangle isn't worth it ATM.
 //
-// # How This Package Is Used By Package Snapper
+// How This Package Is Used By Package Snapper
 //
 // Deserialize a config.List using ListFromConfig().
 // Then it MUST filter the list to only contain hooks for a particular filesystem using
@@ -30,4 +30,5 @@
 // Command hooks make it available in the environment variable ZREPL_DRYRUN.
 //
 // Plan.Report() can be called while Plan.Run() is executing to give an overview of plan execution progress (future use in "zrepl status").
+//
 package hooks

daemon/hooks/hook_type_mysql_lock_tables.go

@@ -17,7 +17,6 @@ import (
 	"github.com/zrepl/zrepl/zfs"
 )
 
-// Hook to implement the following recommmendation from MySQL docs
 // https://dev.mysql.com/doc/mysql-backup-excerpt/5.7/en/backup-methods.html
 //
 // Making Backups Using a File System Snapshot:
@@ -31,8 +30,6 @@ import (
 //    	Unmount the snapshot.
 //
 //	Similar snapshot capabilities may be available in other file systems, such as LVM or ZFS.
-//
-
 type MySQLLockTables struct {
 	errIsFatal  bool
 	connector   sqldriver.Connector

daemon/logging/trace/trace.go

@@ -1,6 +1,6 @@
 // package trace provides activity tracing via ctx through Tasks and Spans
 //
-// # Basic Concepts
+// Basic Concepts
 //
 // Tracing can be used to identify where a piece of code spends its time.
 //
@@ -10,9 +10,11 @@
 // to tech-savvy users (albeit not developers).
 //
 // This package provides the concept of Tasks and Spans to express what activity is happening within an application:
+//
 //  - Neither task nor span is really tangible but instead contained within the context.Context tree
 //  - Tasks represent concurrent activity (i.e. goroutines).
 //  - Spans represent a semantic stack trace within a task.
+//
 // As a consequence, whenever a context is propagated across goroutine boundary, you need to create a child task:
 //
 //   go func(ctx context.Context) {
@@ -37,7 +39,6 @@
 //  }
 //
 // In combination:
-//
 //  ctx, endTask = WithTask(ctx, "copy-dirs")
 //  defer endTask()
 //  for i := range dirs {
@@ -64,7 +65,8 @@
 //
 // Recovering from endSpan() or endTask() panics will corrupt the trace stack and lead to corrupt tracefile output.
 //
-// # Best Practices For Naming Tasks And Spans
+//
+// Best Practices For Naming Tasks And Spans
 //
 // Tasks should always have string constants as names, and must not contain the `#` character. WHy?
 // First, the visualization by chrome://tracing draws a horizontal bar for each task in the trace.
@@ -72,7 +74,8 @@
 // Note that the `#NUM` suffix will be reused if a task has ended, in order to avoid an
 // infinite number of horizontal bars in the visualization.
 //
-// # Chrome-compatible Tracefile Support
+//
+// Chrome-compatible Tracefile Support
 //
 // The activity trace generated by usage of WithTask and WithSpan can be rendered to a JSON output file
 // that can be loaded into chrome://tracing .

daemon/logging/trace/trace_convenience.go

@@ -11,6 +11,8 @@ import (
 // use like this:
 //
 //	defer WithSpanFromStackUpdateCtx(&existingCtx)()
+//
+//
 func WithSpanFromStackUpdateCtx(ctx *context.Context) DoneFunc {
 	childSpanCtx, end := WithSpan(*ctx, getMyCallerOrPanic())
 	*ctx = childSpanCtx

daemon/snapper/periodic.go

@@ -22,7 +22,7 @@ func periodicFromConfig(g *config.Global, fsf zfs.DatasetFilter, in *config.Snap
 	if in.Prefix == "" {
 		return nil, errors.New("prefix must not be empty")
 	}
-	if in.Interval <= 0 {
+	if in.Interval.Duration() <= 0 {
 		return nil, errors.New("interval must be positive")
 	}
 
@@ -32,7 +32,7 @@ func periodicFromConfig(g *config.Global, fsf zfs.DatasetFilter, in *config.Snap
 	}
 
 	args := periodicArgs{
-		interval: in.Interval,
+		interval: in.Interval.Duration(),
 		fsf:      fsf,
 		planArgs: planArgs{
 			prefix: in.Prefix,

endpoint/endpoint_zfs_abstraction.go

@@ -168,7 +168,7 @@ func (s AbstractionTypeSet) String() string {
 	for i := range s {
 		sts = append(sts, string(i))
 	}
-	sort.Strings(sts)
+	sts = sort.StringSlice(sts)
 	return strings.Join(sts, ",")
 }
 

replication/driver/replication_driver.go

@@ -797,7 +797,7 @@ func (a *attempt) errorReport() *errorReport {
 			r.byClass[class] = errs
 		}
 		for _, err := range r.flattened {
-			if neterr, ok := err.Err.(net.Error); ok && neterr.Timeout() {
+			if neterr, ok := err.Err.(net.Error); ok && neterr.Temporary() {
 				putClass(err, errorClassTemporaryConnectivityRelated)
 				continue
 			}

replication/driver/replication_driver_debug.go

@@ -13,7 +13,7 @@ func init() {
 	}
 }
 
-//nolint:deadcode,unused
+//nolint[:deadcode,unused]
 func debug(format string, args ...interface{}) {
 	if debugEnabled {
 		fmt.Fprintf(os.Stderr, "repl: driver: %s\n", fmt.Sprintf(format, args...))
@@ -22,7 +22,7 @@ func debug(format string, args ...interface{}) {
 
 type debugFunc func(format string, args ...interface{})
 
-//nolint:deadcode,unused
+//nolint[:deadcode,unused]
 func debugPrefix(prefixFormat string, prefixFormatArgs ...interface{}) debugFunc {
 	prefix := fmt.Sprintf(prefixFormat, prefixFormatArgs...)
 	return func(format string, args ...interface{}) {

rpc/dataconn/dataconn_debug.go

@@ -14,7 +14,7 @@ func init() {
 	}
 }
 
-//nolint:deadcode,unused
+//nolint[:deadcode,unused]
 func debug(format string, args ...interface{}) {
 	if debugEnabled {
 		fmt.Fprintf(os.Stderr, "rpc/dataconn: %s\n", fmt.Sprintf(format, args...))

rpc/dataconn/heartbeatconn/heartbeatconn.go

@@ -24,9 +24,6 @@ func (e HeartbeatTimeout) Error() string {
 	return "heartbeat timeout"
 }
 
-// This function is deprecated in net.Error and since this
-// function is not involved in .Accept() code path, nothing
-// really needs this method to be here.
 func (e HeartbeatTimeout) Temporary() bool { return true }
 
 func (e HeartbeatTimeout) Timeout() bool { return true }

rpc/dataconn/heartbeatconn/heartbeatconn_debug.go

@@ -13,7 +13,7 @@ func init() {
 	}
 }
 
-//nolint:deadcode,unused
+//nolint[:deadcode,unused]
 func debug(format string, args ...interface{}) {
 	if debugEnabled {
 		fmt.Fprintf(os.Stderr, "rpc/dataconn/heartbeatconn: %s\n", fmt.Sprintf(format, args...))

rpc/dataconn/heartbeatconn/integration_test_variablereceiverate/heartbeatconn_integration_variablereceiverate.go

@@ -22,8 +22,10 @@
 // The fix contained in the commit this message was committed with resets the deadline whenever
 // a heartbeat is received from the server.
 //
+//
 // How to run this integration test:
 //
+//
 //  Terminal 1:
 //     $ ZREPL_RPC_DATACONN_HEARTBEATCONN_DEBUG=1 go run heartbeatconn_integration_variablereceiverate.go  -mode server -addr 127.0.0.1:12345
 //     rpc/dataconn/heartbeatconn: send heartbeat
@@ -39,6 +41,8 @@
 //     rpc/dataconn/heartbeatconn: renew frameconn write timeout returned errT=<nil> err=%!s(<nil>)
 //     rpc/dataconn/heartbeatconn: received heartbeat, resetting write timeout
 //     ...
+//
+//  You should observe
 package main
 
 import (

rpc/dataconn/microbenchmark/microbenchmark.go

@@ -5,10 +5,12 @@
 //   ./microbenchmark -appmode server  | pv -r > /dev/null
 //   ./microbenchmark -appmode client -direction recv < /dev/zero
 //
+//
 // Without the overhead of pipes (just protocol performance, mostly useful with perf bc no bw measurement)
 //
 //   ./microbenchmark -appmode client -direction recv -devnoopWriter -devnoopReader
 //   ./microbenchmark -appmode server -devnoopReader -devnoopWriter
+//
 package main
 
 import (

rpc/dataconn/stream/stream.go

@@ -29,7 +29,7 @@ func WithLogger(ctx context.Context, log Logger) context.Context {
 	return context.WithValue(ctx, contextKeyLogger, log)
 }
 
-//nolint:deadcode,unused
+//nolint[:deadcode,unused]
 func getLog(ctx context.Context) Logger {
 	log, ok := ctx.Value(contextKeyLogger).(Logger)
 	if !ok {
@@ -181,19 +181,23 @@ func (e *ReadStreamError) Error() string {
 
 var _ net.Error = &ReadStreamError{}
 
-func (e ReadStreamError) Timeout() bool {
+func (e ReadStreamError) netErr() net.Error {
 	if netErr, ok := e.Err.(net.Error); ok {
+		return netErr
+	}
+	return nil
+}
+
+func (e ReadStreamError) Timeout() bool {
+	if netErr := e.netErr(); netErr != nil {
 		return netErr.Timeout()
 	}
 	return false
 }
 
-// This function is deprecated in net.Error and since this
-// function is not involved in .Accept() code path, nothing
-// really needs this method to be here.
 func (e ReadStreamError) Temporary() bool {
-	if te, ok := e.Err.(interface{ Temporary() bool }); ok {
+	if netErr := e.netErr(); netErr != nil {
-		return te.Temporary()
+		return netErr.Temporary()
 	}
 	return false
 }

rpc/dataconn/stream/stream_conn.go

@@ -232,12 +232,7 @@ var _ net.Error = (*closeStateErrConnectionClosed)(nil)
 func (e *closeStateErrConnectionClosed) Error() string {
 	return "connection closed"
 }
-
 func (e *closeStateErrConnectionClosed) Timeout() bool   { return false }
-
-// This function is deprecated in net.Error and since this
-// function is not involved in .Accept() code path, nothing
-// really needs this method to be here.
 func (e *closeStateErrConnectionClosed) Temporary() bool { return false }
 
 func (s *closeState) CloseEntry() error {

rpc/dataconn/stream/stream_debug.go

@@ -13,7 +13,7 @@ func init() {
 	}
 }
 
-//nolint:deadcode,unused
+//nolint[:deadcode,unused]
 func debug(format string, args ...interface{}) {
 	if debugEnabled {
 		fmt.Fprintf(os.Stderr, "rpc/dataconn/stream: %s\n", fmt.Sprintf(format, args...))

rpc/netadaptor/authlistener_netlistener_adaptor.go

@@ -1,5 +1,25 @@
 // Package netadaptor implements an adaptor from
 // transport.AuthenticatedListener to net.Listener.
+//
+// In contrast to transport.AuthenticatedListener,
+// net.Listener is commonly expected (e.g. by net/http.Server.Serve),
+// to return errors that fulfill the Temporary interface:
+//   interface Temporary { Temporary() bool }
+// Common behavior of packages consuming net.Listener is to return
+// from the serve-loop if an error returned by Accept is not Temporary,
+// i.e., does not implement the interface or is !net.Error.Temporary().
+//
+// The zrepl transport infrastructure was written with the
+// idea that Accept() may return any kind of error, and the consumer
+// would just log the error and continue calling Accept().
+// We have to adapt these listeners' behavior to the expectations
+// of net/http.Server.
+//
+// Hence, Listener does not return an error at all but blocks the
+// caller of Accept() until we get a (successfully authenticated)
+// connection without errors from the transport.
+// Accept errors returned from the transport are logged as errors
+// to the logger passed on initialization.
 package netadaptor
 
 import (

rpc/rpc_client.go

@@ -188,8 +188,8 @@ func (c *Client) WaitForConnectivity(ctx context.Context) error {
 			data, dataErr := c.dataClient.ReqPing(ctx, &req)
 			// dataClient uses transport.Connecter, which doesn't expose WaitForReady(true)
 			// => we need to mask dial timeouts
-			if err, ok := dataErr.(interface{ Timeout() bool }); ok && err.Timeout() {
+			if err, ok := dataErr.(interface{ Temporary() bool }); ok && err.Temporary() {
-				// Rate-limit pings here in case Timeout() == true is a mis-classification
+				// Rate-limit pings here in case Temporary() is a mis-classification
 				// or returns immediately (this is a tight loop in that case)
 				// TODO keep this in lockstep with controlClient
 				// 		=> don't use FailFast for control, but check that both control and data worked

rpc/rpc_debug.go

@@ -14,7 +14,7 @@ func init() {
 	}
 }
 
-//nolint:deadcode,unused
+//nolint[:deadcode,unused]
 func debug(format string, args ...interface{}) {
 	if debugEnabled {
 		fmt.Fprintf(os.Stderr, "rpc: %s\n", fmt.Sprintf(format, args...))

rpc/rpc_doc.go

@@ -3,7 +3,7 @@
 // The zrepl documentation refers to the client as the
 // `active side` and to the server as the `passive side`.
 //
-// # Design Considerations
+// Design Considerations
 //
 // zrepl has non-standard requirements to remote procedure calls (RPC):
 // whereas the coordination of replication (the planning phase) mostly
@@ -35,7 +35,7 @@
 //
 // Hence, this package attempts to combine the best of both worlds:
 //
-// # GRPC for Coordination and Dataconn for Bulk Data Transfer
+// GRPC for Coordination and Dataconn for Bulk Data Transfer
 //
 // This package's Client uses its transport.Connecter to maintain
 // separate control and data connections to the Server.
@@ -107,6 +107,8 @@
 //                    |  Handler   |
 //                    +------------+
 //           (usually endpoint.{Sender,Receiver})
+//
+//
 package rpc
 
 // edit trick for the ASCII art above:

rpc/rpc_logging.go

@@ -9,7 +9,7 @@ import (
 
 type Logger = logger.Logger
 
-// All fields must be non-nil
+/// All fields must be non-nil
 type Loggers struct {
 	General Logger
 	Control Logger

rpc/versionhandshake/versionhandshake.go

@@ -33,47 +33,8 @@ var _ net.Error = &HandshakeError{}
 
 func (e HandshakeError) Error() string { return e.msg }
 
-// When a net.Listener.Accept() returns an error, the server must
+// Like with net.OpErr (Go issue 6163), a client failing to handshake
-// decide whether to retry calling Accept() or not.
+// should be a temporary Accept error toward the Listener .
-// On some platforms (e.g., Linux), Accept() can return errors
-// related to the specific protocol connection that was supposed
-// to be returned as asocket FD. Obviously, we want to ignore,
-// maybe log, those errors and retry Accept() immediately to
-// serve other connections.
-// But there are also conditions where we get Accept() errors because
-// the process has run out of file descriptors. In that case, retrying
-// won't help. We need to close some file descriptor to make progress.
-// Note that there could be lots of open file descriptors because we
-// have accepted, and not yet closed, lots of connections in the past.
-// And then, of course there can be errors where we just want
-// to return, e.g., if there's a programming error and we're getting
-// an EBADFD or whatever.
-//
-// So, the serve loops in net/http.Server.Serve() or gRPC's server.Serve()
-// must inspect the error and decide what to do.
-// The vehicle for this is the
-//
-//	interface { Temporary() bool }
-//
-// Behavior in both of the aforementioned Serve() loops:
-//
-//   - if the error doesn't implement the interface, stop serving and return
-//   - `Temporary() == true`: retry with back-off
-//   - `Temporary() == false`: stop serving and return
-//
-// So, to make this package's HandshakeListener work with these
-// Serve() loops, we return Temporary() == true if the handshake fails.
-// In the aforementioned categories, that's the case of a per-connection
-// protocol error.
-//
-// Note: the net.Error interface has deprecated the Temporary() method
-// in go.dev/issue/45729, but there is no replacement for users of .Accept().
-// Existing users of .Accept() continue to check for the interface.
-// So, we need to continue supporting Temporary() until there's a different
-// mechanism for serve loops to decide whether to retry or not.
-// The following mailing list post proposes to eliminate the retries
-// completely, but it seems like the effort has stalled.
-// https://groups.google.com/g/golang-nuts/c/-JcZzOkyqYI/m/xwaZzjCgAwAJ
 func (e HandshakeError) Temporary() bool {
 	if e.isAcceptError {
 		return true

util/chainlock/chainlock.go

@@ -9,6 +9,7 @@
 //	     defer a.l.Unlock().Lock()
 //	     fssesDone.Wait()
 //	 }()
+//
 package chainlock
 
 import "sync"

util/nodefault/nodefault.go

@@ -28,4 +28,5 @@
 //  f(Config{}) // crashes
 //
 //  f Config{ CriticalSetting: &nodefault.Bool{B: false}} // doesn't crash
+//
 package nodefault

zfs/namecheck.go

@@ -53,6 +53,7 @@ var componentValidChar = regexp.MustCompile(`^[0-9a-zA-Z-_\.: ]+$`)
 // characters:
 //
 //	[-_.: ]
+//
 func ComponentNamecheck(datasetPathComponent string) error {
 	if len(datasetPathComponent) == 0 {
 		return fmt.Errorf("path component must not be empty")

zfs/zfs.go

@@ -1022,10 +1022,8 @@ func (s *DrySendInfo) unmarshalZFSOutput(output []byte) (err error) {
 }
 
 // unmarshal info line, looks like this:
-//
 //   full	zroot/test/a@1	5389768
 //   incremental	zroot/test/a@1	zroot/test/a@2	5383936
-//
 // => see test cases
 func (s *DrySendInfo) unmarshalInfoLine(l string) (regexMatched bool, err error) {
 
@@ -1857,6 +1855,7 @@ var ErrBookmarkCloningNotSupported = fmt.Errorf("bookmark cloning feature is not
 // unless a bookmark with the name `bookmark` exists and has the same idenitty (zfs.FilesystemVersionEqualIdentity)
 //
 // v must be validated by the caller
+//
 func ZFSBookmark(ctx context.Context, fs string, v FilesystemVersion, bookmark string) (bm FilesystemVersion, err error) {
 
 	bm = FilesystemVersion{

zfs/zfs_debug.go

@@ -13,7 +13,7 @@ func init() {
 	}
 }
 
-//nolint:deadcode,unused
+//nolint[:deadcode,unused]
 func debug(format string, args ...interface{}) {
 	if debugEnabled {
 		fmt.Fprintf(os.Stderr, "zfs: %s\n", fmt.Sprintf(format, args...))