[Task]: Go SDK: coder.RegisterCoder needs more precise docstring

[gonzojive]

What would you like to happen?

coder.RegisterCoder should specify the accepted forms of the "enc" and "dec" arguments. Same for NewCustomCoder.

// RegisterCoder registers a user defined coder for a given type, and will
// be used if there is no beam coder for that type. Must be called prior to beam.Init(),
// preferably in an init() function.
//
// Coders are encoder and decoder pairs, and operate around []bytes.
//
// The coder used for a given type follows this ordering:
//   1. Coders for Known Beam types.
//   2. Coders registered for specific types
//   3. Coders registered for interfaces types
//   4. Default coder (JSON)
//
// Types of kind Interface, are handled specially by the registry, so they may be iterated
// over to check if element types implement them.
//
// Repeated registrations of the same type overrides prior ones.

// NewCustomCoder creates a coder for the supplied parameters defining a
// particular encoding strategy.

The implementation seems to rely on this code:

func validateEncoder(t reflect.Type, encode interface{}) error {
	// Check if it uses the real type in question.
	if err := funcx.Satisfy(encode, funcx.Replace(encodeSig, typex.TType, t)); err != nil {
		return errors.WithContext(err, "validateEncoder: validating signature")
	}
	// TODO(lostluck): 2019.02.03 - Determine if there are encode allocation bottlenecks.
	return nil
}

func validateDecoder(t reflect.Type, decode interface{}) error {
	// Check if it uses the real type in question.
	if err := funcx.Satisfy(decode, funcx.Replace(decodeSig, typex.TType, t)); err != nil {
		return errors.WithContext(err, "validateDecoder: validating signature")
	}
	// TODO(lostluck): 2019.02.03 - Expand cases to avoid []byte -> interface{} conversion
	// in exec, & a beam Decoder interface.
	return nil
}

Issue Priority

Priority: 2

Issue Component

Component: sdk-go

[gonzojive]

Aha, here is what I'm now seeing in the docs. This should at least be linked from the mentioned functions.

// CustomCoder contains possibly untyped encode/decode user functions that are
// type-bound at runtime. Universal coders can thus be used for many different
// types, but each CustomCoder instance will be bound to a specific type.
type CustomCoder struct {
	// Name is the coder name. Informational only.
	Name string
	// Type is the underlying concrete type that is being coded. It is
	// available to Enc and Dec. It must be a concrete type.
	Type reflect.Type

	// Enc is the encoding function : T -> []byte. It may optionally take a
	// reflect.Type parameter and return an error as well.
	Enc *funcx.Fn
	// Dec is the decoding function: []byte -> T. It may optionally take a
	// reflect.Type parameter and return an error as well.
	Dec *funcx.Fn

	ID string // (optional) This coder's ID if translated from a pipeline proto.
}

[lostluck]

Made it a task instead of a new feature, as it's updating the documentation.

My main regret from that API is not making it a factory function. As it stands they need to be pure functions, or global state that needs to be made threadsafe. There's no other option.

Specifically, it's not possible to have "per coder" state, it's either no state or global, which means users have to ensure any state is thread safe, unlike everything else which allows bundle scoped state, which avoids the need for thread safety since an instance is only used by a single bundle at a time.