Create a MockCluster within Go for testing #729

SourceFellows · 2022-01-14T14:09:20Z

Hi,

this PR exposes the MockCluster from librdkafka to Golang.
You are able to start a MockCluster with:

kafka.NewMockCluster(1)

and read the corresponding bootstrap.servers property via:

broker := mockCluster.Bootstrapservers()

Remote communication is also possible.

An example is also added.

Thanks,
Kristian

edenhill

This is great stuff!

examples/mockserver_example/mockserver.go

kafka/mockcluster.go

kafka/librdkafka_vendor/rdkafka_mock.h

kafka/mockcluster.go

kkoehler · 2022-02-19T08:05:47Z

Hi @edenhill,

still something missing?
I can't figure out what does "changes requested" mean.... is there any work needed on my side? ;-)
Thanks,
Kristian

gstilwellDO · 2022-03-01T17:27:38Z

Hi @edenhill,

still something missing? I can't figure out what does "changes requested" mean.... is there any work needed on my side? ;-) Thanks, Kristian

Indeed, it would be great to get this merged! I've got a project itching to use it :)

jliunyu · 2022-03-05T07:10:54Z

Hi @edenhill,

still something missing? I can't figure out what does "changes requested" mean.... is there any work needed on my side? ;-) Thanks, Kristian

Let's wait for a CI test before the merge.

garrybodsworth · 2022-03-10T11:49:33Z

kafka/mockcluster.go

+
+	mc.cConf = C.rd_kafka_conf_new()
+
+	mc.rk = C.rd_kafka_new(C.RD_KAFKA_PRODUCER, mc.cConf, cErrstr, 256)


There is an issue here when Close() is called which causes a crash.

In the documentation for rd_kafka_new it says:

conf is an optional struct created with rd_kafka_conf_new() that will be used instead of the default configuration. The conf object is freed by this function on success and must not be used or destroyed by the application sub-sequently. See rd_kafka_conf_set() et.al for more information.

This means that on success there should be no calls to C.rd_kafka_conf_destroy(mc.cConf) which is done in Close()

Ok. I removed the call to the conf_destroy function to prevent problems ;-) Thanks

edenhill · 2022-03-22T08:38:58Z

examples/mockserver_example/mockserver.go

@@ -0,0 +1,100 @@
+package main
+


Can we rename this to mockcluster_example instead, to match the API naming?

edenhill · 2022-03-22T08:40:37Z

examples/mockserver_example/mockserver.go

+
+	c, err := kafka.NewConsumer(&kafka.ConfigMap{
+		"bootstrap.servers": broker,
+		// Avoid connecting to IPv6 brokers:


Remove this comment block

edenhill · 2022-03-22T08:43:13Z

kafka/mockcluster.go

+ * limitations under the License.
+ */
+
+package kafka


Put the package as the first line of the file

edenhill · 2022-03-22T08:43:25Z

kafka/mockcluster.go

+
+package kafka
+
+import "C"


Duplicate import, remove this one.

edenhill · 2022-03-22T08:43:38Z

kafka/mockcluster.go

+
+package kafka
+
+import "C"


Duplicate import, remove this one.

edenhill · 2022-03-22T08:46:20Z

kafka/mockcluster.go

+// @remark This is an experimental public API that is NOT covered by the
+//         librdkafka API or ABI stability guarantees.
+//
+// @warning THIS IS AN EXPERIMENTAL API, SUBJECT TO CHANGE OR REMOVAL.


Suggested change

// @warning THIS IS AN EXPERIMENTAL API, SUBJECT TO CHANGE OR REMOVAL.

// Warning: THIS IS AN EXPERIMENTAL API, SUBJECT TO CHANGE OR REMOVAL.

edenhill · 2022-03-22T10:02:30Z

kafka/mockcluster.go

+	cErrstr := (*C.char)(C.malloc(C.size_t(256)))
+	defer C.free(unsafe.Pointer(cErrstr))
+
+	mc.cConf = C.rd_kafka_conf_new()


No need to store this on the MockCluster object, it is only used in this function

removed the config reference from MockCluster

edenhill · 2022-03-22T10:02:42Z

kafka/mockcluster.go

+
+	mc := &MockCluster{}
+
+	cErrstr := (*C.char)(C.malloc(C.size_t(256)))


Make this 512 to avoid truncation

edenhill · 2022-03-22T10:03:12Z

kafka/mockcluster.go

+	mc.mcluster = C.rd_kafka_mock_cluster_new(mc.rk, C.int(brokerCount))
+	if mc.mcluster == nil {
+		C.rd_kafka_destroy(mc.rk)
+		C.rd_kafka_conf_destroy(mc.cConf)


Remove this line, the cConf is already freed by the successful rd_kafka_new call

ok. removed the call

edenhill · 2022-03-22T10:03:40Z

kafka/mockcluster.go

+}
+
+// Bootstrapservers returns the bootstrap.servers property for this MockCluster
+func (mc *MockCluster) Bootstrapservers() string {


Rename to BootstrapServers()

kkoehler · 2022-03-22T11:08:07Z

Ok. Sorry for the single commits. @edenhill changed the impl according to your suggestion

edenhill

LGTM!

edenhill · 2022-03-23T10:06:32Z

I can't merge this just yet because we need you to sign the CLA, and we're in the midst of the process of switching CLA signing solutions.
Will ping you when that is in place, thanks for your work on this and your patience!

CLAassistant · 2022-04-01T15:56:25Z

All committers have signed the CLA.

Co-authored-by: Magnus Edenhill <magnus@edenhill.se>

…cluster_t

jeffwidman · 2022-04-07T21:49:59Z

@edenhill it looks like this is ready to go including CLA... any chance of a merge with it? We're itching to use it...

Also, many thanks for all the hard work @SourceFellows!

Update: I was thinking even if it's on master but not yet tagged, that'd be helpful as we can pin to the specific commit. But it looks like there's no support for go modules yet?? #553

edenhill · 2022-04-08T09:56:14Z

Thank you for this!

jeffwidman · 2022-04-08T16:07:16Z

Thanks for the quick merge!

edenhill · 2022-04-08T16:19:14Z

I think a blog post how to use this as part of application testing would be fantastic.. just saying..saying

kkoehler · 2022-04-08T16:29:17Z

hearing that ;-) i started but not finished yet... going soon

@edenhill

…confluentinc#729) * Added MockCluster implementation which can be used for testing * adjust documentation for mock cluster * Added example for MockCluster * Fixed naming Co-authored-by: Magnus Edenhill <magnus@edenhill.se> * fixed typo Co-authored-by: Magnus Edenhill <magnus@edenhill.se> * removed Co-authored-by: Magnus Edenhill <magnus@edenhill.se> * fixed comment Co-authored-by: Magnus Edenhill <magnus@edenhill.se> * adjust documentation Co-authored-by: Magnus Edenhill <magnus@edenhill.se> * adjustments regarding comments from Magnus * fixed cleanup code for kafka configuration * fixed destroy for C references * fixed spaces * removed call to `C.rd_kafka_conf_destroy(mc.cConf)` in close * added docu for MockCluster type * moved package statement into first line * adjustments according to @edenhill comments * Update kafka/mockcluster.go Co-authored-by: Magnus Edenhill <magnus@edenhill.se> * removed duplicate import * removed the remark as suggested from @edelhill * changed typedef from struct_rd_kafka_mock_cluster_s to rd_kafka_mock_cluster_t Co-authored-by: Kristian Köhler <github@kkoehler.com> Co-authored-by: Magnus Edenhill <magnus@edenhill.se> Co-authored-by: Kristian Köhler <kkoehler@users.noreply.github.com>

SourceFellows mentioned this pull request Jan 17, 2022

Need a way to extract bootstrap servers after mock servers are created #689

Closed

edenhill suggested changes Jan 17, 2022

View reviewed changes

edenhill reviewed Jan 17, 2022

View reviewed changes

kafka/mockcluster.go Outdated Show resolved Hide resolved

garrybodsworth reviewed Mar 10, 2022

View reviewed changes

edenhill suggested changes Mar 22, 2022

View reviewed changes

edenhill approved these changes Mar 22, 2022

View reviewed changes

kkoehler and others added 18 commits April 4, 2022 06:24

Added MockCluster implementation which can be used for testing

1046d12

adjust documentation for mock cluster

6820da3

Added example for MockCluster

41f58df

Fixed naming

c161fd9

Co-authored-by: Magnus Edenhill <magnus@edenhill.se>

fixed typo

44c5fb5

Co-authored-by: Magnus Edenhill <magnus@edenhill.se>

removed

52be16e

Co-authored-by: Magnus Edenhill <magnus@edenhill.se>

fixed comment

0849554

Co-authored-by: Magnus Edenhill <magnus@edenhill.se>

adjust documentation

05c0e81

Co-authored-by: Magnus Edenhill <magnus@edenhill.se>

adjustments regarding comments from Magnus

4414d8b

fixed cleanup code for kafka configuration

8cb4c39

fixed destroy for C references

716ba61

fixed spaces

7f8f7e8

removed call to C.rd_kafka_conf_destroy(mc.cConf) in close

debc243

added docu for MockCluster type

2f111f8

moved package statement into first line

36d0b06

adjustments according to @edenhill comments

abc6aaf

Update kafka/mockcluster.go

95bcc36

Co-authored-by: Magnus Edenhill <magnus@edenhill.se>

removed duplicate import

0d6a89f

kkoehler added 2 commits April 4, 2022 06:25

removed the remark as suggested from @edelhill

ade8aa1

changed typedef from struct_rd_kafka_mock_cluster_s to rd_kafka_mock_…

9d526f9

…cluster_t

kkoehler force-pushed the master branch from e44f2f3 to 9d526f9 Compare April 4, 2022 04:26

edenhill merged commit f9fce7a into confluentinc:master Apr 8, 2022


		mc.cConf = C.rd_kafka_conf_new()

		mc.rk = C.rd_kafka_new(C.RD_KAFKA_PRODUCER, mc.cConf, cErrstr, 256)

	// @warning THIS IS AN EXPERIMENTAL API, SUBJECT TO CHANGE OR REMOVAL.
	// Warning: THIS IS AN EXPERIMENTAL API, SUBJECT TO CHANGE OR REMOVAL.


		mc := &MockCluster{}

		cErrstr := (*C.char)(C.malloc(C.size_t(256)))

Create a MockCluster within Go for testing #729

Create a MockCluster within Go for testing #729

Conversation

SourceFellows commented Jan 14, 2022

edenhill left a comment

Choose a reason for hiding this comment

kkoehler commented Feb 19, 2022

gstilwellDO commented Mar 1, 2022

jliunyu commented Mar 5, 2022

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

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

Choose a reason for hiding this comment

kkoehler commented Mar 22, 2022

edenhill left a comment

Choose a reason for hiding this comment

edenhill commented Mar 23, 2022

CLAassistant commented Apr 1, 2022 • edited

jeffwidman commented Apr 7, 2022 • edited

edenhill commented Apr 8, 2022

jeffwidman commented Apr 8, 2022

edenhill commented Apr 8, 2022

kkoehler commented Apr 8, 2022

CLAassistant commented Apr 1, 2022 •

edited

jeffwidman commented Apr 7, 2022 •

edited