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

Un-deprecate NewWriter #706

Open
alexec opened this issue Jul 15, 2021 · 11 comments
Open

Un-deprecate NewWriter #706

alexec opened this issue Jul 15, 2021 · 11 comments
Assignees

Comments

@alexec
Copy link

alexec commented Jul 15, 2021

Creating a kafka.Writer directly seems to be much more complex that calling NewWriter. For example, it puts into place sensible defaults, makes setting up the kafka.Dialer for TLS really easy. I propose either:

  1. Un-deprecate it - it's really useful.
  2. Something else?
@achille-roussel
Copy link
Contributor

Hello @alexec.

The kafka.Writer type already uses default values for fields that aren't being set, which are documented, for example:

	// The balancer used to distribute messages across partitions.
	//
	// The default is to use a round-robin distribution.
	Balancer Balancer

Your program does not need to assign every single field of the kafka.Writer type to successfully use it.

Would you be able to provide examples where using the kafka.Writer increases complexity of the code over kafka.NewWriter? These would be useful in helping guide the design decisions.

@alexec
Copy link
Author

alexec commented Jul 15, 2021

The specific case I found was setting up TLS, kafka.Writer{} does not have Dialer field. Instead I need to create a Transport. Was not clear how to do so.

@alexec
Copy link
Author

alexec commented Jul 15, 2021

Aside:

I think I've mentioned I want both speed and reliability, but reliability trumps speed be default. I think I need to provide a knob so users can choose between them.

I believe that main way to do this would be to expose the BatchTimeout and Async flags to the user. Then they can choose their reliability/speed trade-off.

Does this sound correct?

@nikola-sever-syntio
Copy link

nikola-sever-syntio commented Jul 19, 2021

Hello,

This is also my concern. We also want to support TLS encryption but now in default Writer type there is, as mentioned before, no Dialer support which is used to implement TLS. And with NewWriter type will lose support in v1. Will there be TLS support implemented into Writer type?

@kishaningithub
Copy link
Contributor

@nikola-sever-syntio / @alexec An example of configuring dialer with a second timeout and TLS

tlsConfig := // construct tls config

kafkaWriter.Transport = &kafka.Transport{
	Dial: (&net.Dialer{
		Timeout: 3 * time.Second,
	}).DialContext,
	TLS: tlsConfig,
}

@nikola-sever-syntio
Copy link

@kishaningithub thank you

@achille-roussel
Copy link
Contributor

@alexec

The specific case I found was setting up TLS, kafka.Writer{} does not have Dialer field. Instead I need to create a Transport. Was not clear how to do so.

The writer uses kafka.DefaultTransport by default. You can configure the dialer function there, or create an alternative transport like @kishaningithub suggested. This designed is mirrored after the net/http package.

I believe that main way to do this would be to expose the BatchTimeout and Async flags to the user. Then they can choose their reliability/speed trade-off.

The kafka.Writer type already expose BatchTimeout and Async, so I would assume you are referring to something else. Could you be more specific about where you would like to see these configuration options added in?

@adrian-mcmichael
Copy link

adrian-mcmichael commented Jul 26, 2021

To add to the sentiment above, I'd say the pattern you have now allows teams to wrap configuration and provide context local defaults more easily via your WriterConfig struct. To reference a comment on the Writer:

// Methods of Writer are safe to use concurrently from multiple goroutines, // however the writer configuration should not be modified after first use.

This becomes a non-issue if you keep the WriterConfig, as from an engineer's perspective there is less of an expectation that altering a field on a config after its passed to a constructor would have an effect whereas making changes to a field directly on a Writer is something that there would be a viable change to the way it worked.

As it stands at the moment, to apply local contextual config on behalf of teams I need to mirror each and everyone of your config variables if you were to remove the WriterConfig. Whereas if you keep it I can provide context local sensible defaults (cluster addresses etc) whilst still providing deep config for power users, without needing to create a mirrored setting for each Writer setting.

@achille-roussel
Copy link
Contributor

achille-roussel commented Jul 26, 2021

Thanks for the context @adrian-mcmichael, these are interesting points that we'll take into account.

@calmera
Copy link

calmera commented Aug 31, 2021

There also seems to be an inconsistency in the api since a reader config requires a Dialer while writers require a transport. Or am I missing something?

@achille-roussel
Copy link
Contributor

You are correct, this is an inconsistency we are planning to resolve in upcoming versions of the package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants