[Feature] Enable Nats-server as a optional MQ for Milvus #24445

chyezh · 2023-05-26T08:34:33Z

Enable Nats-server as a optional MQ for Milvus.

prototype and unit test by @yiwangdr #23606
more unit test, bug fix and pass e2e.
Related issue: #23611

mergify · 2023-05-26T08:42:53Z

@chyezh ut workflow job failed, comment rerun ut can trigger the job again.

mergify · 2023-05-26T09:01:31Z

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

mergify · 2023-05-26T10:11:21Z

@chyezh ut workflow job failed, comment rerun ut can trigger the job again.

mergify · 2023-05-26T10:31:42Z

@chyezh ut workflow job failed, comment rerun ut can trigger the job again.

codecov · 2023-05-26T11:09:51Z

Codecov Report

Merging #24445 (e9f22af) into master (852e5ce) will increase coverage by 0.45%.
The diff coverage is 82.24%.

@@            Coverage Diff             @@
##           master   #24445      +/-   ##
==========================================
+ Coverage   82.18%   82.63%   +0.45%     
==========================================
  Files         777      776       -1     
  Lines      103129   102013    -1116     
==========================================
- Hits        84756    84301     -455     
+ Misses      15358    14678     -680     
- Partials     3015     3034      +19

Impacted Files	Coverage Δ
pkg/mq/msgstream/common_mq_factory.go	`57.14% <57.14%> (ø)`
internal/util/dependency/factory.go	`64.38% <58.13%> (+24.38%)`	⬆️
internal/mq/msgstream/mq_factory.go	`75.00% <75.00%> (+13.46%)`	⬆️
pkg/mq/msgstream/mqwrapper/nmq/nmq_consumer.go	`79.78% <79.78%> (ø)`
pkg/mq/msgstream/mqwrapper/nmq/nmq_producer.go	`80.95% <80.95%> (ø)`
pkg/mq/msgstream/mqwrapper/nmq/nmq_client.go	`81.25% <81.25%> (ø)`
pkg/mq/msgstream/mqwrapper/nmq/nmq_server.go	`85.36% <85.36%> (ø)`
pkg/mq/msgstream/mqwrapper/nmq/nmq_message.go	`86.95% <86.95%> (ø)`
pkg/util/paramtable/service_param.go	`98.45% <97.89%> (-0.10%)`	⬇️
internal/mq/msgstream/mqwrapper/rmq/rmq_client.go	`92.30% <100.00%> (+0.81%)`	⬆️
... and 2 more

... and 217 files with indirect coverage changes

mergify · 2023-05-27T04:01:49Z

@chyezh ut workflow job failed, comment rerun ut can trigger the job again.

mergify · 2023-05-27T05:03:40Z

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

chyezh · 2023-05-27T05:06:33Z

/run-cpu-e2e

internal/mq/msgstream/mq_factory.go

internal/mq/msgstream/mqwrapper/rmq/rocksmq_msgstream_test.go

internal/mq/mqimpl/natsmq/server/global_nmq_test.go

internal/mq/mqimpl/natsmq/server/global_nmq.go

jaime0815 · 2023-05-29T08:48:57Z

internal/mq/mqimpl/natsmq/server/global_nmq.go

+		// Wait for server to be ready for connections
+		if !Nmq.ReadyForConnections(initializeTimeout) {
+			finalErr = fmt.Errorf("nmq is not ready with timeout %s", initializeTimeout)
+			log.Warn("nmq is not ready", zap.Duration("timeout", initializeTimeout), zap.Reflect("options", opts))


It might cause some exception behaviors if nmq server is not ready and skip it within a singleton.

log.Fatal is likely better here

new commit changed:
log.Fatal with error field is added into MustInitNatsMQ

jaime0815 · 2023-05-29T09:06:57Z

internal/util/dependency/factory.go


-	f.msgStreamFactory = f.initMQRemoteService(params)
+func (f *DefaultFactory) initMQ(standalone bool, params *paramtable.ComponentParam) error {
+	mqType := mustSelectMQType(standalone, params.MQCfg.Type.GetValue(), mqEnable{params.RocksmqEnable(), params.NatsmqEnable(), params.PulsarEnable(), params.KafkaEnable()})


Just invoke validateMQType to check parameters, then create msgstream factory, it is more simple.

Some rule is applied if mqType = default, validateMQType is not enough.

jaime0815 · 2023-05-29T09:07:39Z

internal/util/dependency/factory.go

+		return errors.Newf("mq.type %s is invalid", mqType)
+	}
+	if !standalone && (mqType == mqTypeRocksmq || mqType == mqTypeNatsmq) {
+		return errors.Newf("mq %s is only valid in standalone mode")


mq.type is the field of milvus.yaml.

jaime0815 · 2023-05-29T09:08:14Z

pkg/util/paramtable/service_param.go

+func (p *MQConfig) Init(base *BaseTable) {
+	p.Type = ParamItem{
+		Key:          "mq.type",
+		Version:      "2.2.0",


should be 2.3.0?

new commit changed:
all field of this commit in yaml is changed to 2.3.0

jaime0815 · 2023-05-29T09:09:29Z

pkg/util/paramtable/service_param.go

+}
+
+// Init sets up a new NatsmqConfig instance using the provided BaseTable
+func (r *NatsmqConfig) Init(base *BaseTable) {


Set version to 2.3.0 for all new parameters

OOC, where do we read this "version" field?

It just indicates this parameter bringing into Milvus with this version.

In that case, we probably don't need to have it in every single param. A global param is good enough.

For this pr, we can have a local/global variable instead of specifying "2.2.0"/"2.3.0" per param.

jaime0815 · 2023-05-29T09:11:38Z

pkg/util/paramtable/service_param.go

+	r.Server.MaxMemoryStore = ParamItem{
+		Key:          "natsmq.server.maxMemoryStore",
+		Version:      "2.2.0",
+		DefaultValue: "536870912",


new commit changed:
memory storage is removed, it's not used in natsmq.

jaime0815 · 2023-05-29T09:14:53Z

pkg/util/paramtable/service_param.go

+	r.Server.MaxFileStore = ParamItem{
+		Key:          "natsmq.server.maxFileStore",
+		Version:      "2.2.0",
+		DefaultValue: "68719476736",


is it too large?

new commit changed:
16G.

chyezh · 2023-05-30T08:39:23Z

move nats implementation into pkg module

commit changed:

move nats implementation into pkg module.
recover some orignal code of rmq implementation.
review is replied, most of the suggestion is applied.

mergify · 2023-05-30T10:21:52Z

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

chyezh · 2023-05-30T11:53:59Z

/run-cpu-e2e

yiwangdr · 2023-05-30T17:03:52Z

move nats implementation into pkg module

commit changed:

move nats implementation into pkg module.

recover some orignal code of rmq implementation.

review is replied, most of the suggestion is applied.

Thanks, is it ready for review now?

chyezh · 2023-05-31T03:46:26Z

/assign @czs007

pkg/util/paramtable/service_param.go

pkg/mq/msgstream/mqwrapper/nmq/nmq_consumer.go

Signed-off-by: yiwangdr <yiwangdr@gmail.com> bug fixup, configurable natsmq, add unittest, pass e2e. Signed-off-by: chyezh <ye.zhen@zilliz.com> move natsmq to pkg project Signed-off-by: chyezh <ye.zhen@zilliz.com>

mergify · 2023-06-06T12:46:40Z

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

chyezh · 2023-06-06T12:58:18Z

/run-cpu-e2e

mergify · 2023-06-06T13:10:31Z

@chyezh E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

chyezh · 2023-06-06T13:48:14Z

/run-cpu-e2e

czs007 · 2023-06-07T01:59:30Z

/approve
/lgtm

sre-ci-robot · 2023-06-07T01:59:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chyezh, czs007, yiwangdr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [czs007]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sre-ci-robot added the area/dependency Pull requests that update a dependency file label May 26, 2023

sre-ci-robot requested review from bigsheeper, congqixia and godchen0212 May 26, 2023 08:34

sre-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines. label May 26, 2023

mergify bot added the dco-passed DCO check passed. label May 26, 2023

chyezh force-pushed the feature_enable_natsmq branch from bc5370b to bf349a9 Compare May 26, 2023 10:04

chyezh force-pushed the feature_enable_natsmq branch from bf349a9 to f543ff9 Compare May 26, 2023 10:20

chyezh force-pushed the feature_enable_natsmq branch from f543ff9 to 31eebdf Compare May 27, 2023 03:44

chyezh force-pushed the feature_enable_natsmq branch from 31eebdf to 49577f8 Compare May 27, 2023 04:42

mergify bot added the ci-passed label May 27, 2023

chyezh requested review from jaime0815 and yiwangdr May 29, 2023 01:58

chyezh commented May 29, 2023

View reviewed changes

internal/mq/msgstream/mq_factory.go Show resolved Hide resolved

internal/mq/msgstream/mqwrapper/rmq/rocksmq_msgstream_test.go Show resolved Hide resolved

internal/mq/mqimpl/natsmq/server/global_nmq_test.go Outdated Show resolved Hide resolved

jaime0815 reviewed May 29, 2023

View reviewed changes

internal/mq/mqimpl/natsmq/server/global_nmq.go Outdated Show resolved Hide resolved

jaime0815 reviewed May 29, 2023

View reviewed changes

chyezh force-pushed the feature_enable_natsmq branch from 8431f07 to f26e1b8 Compare May 30, 2023 09:49

mergify bot added the ci-passed label May 30, 2023

chyezh force-pushed the feature_enable_natsmq branch from f26e1b8 to c6b4e89 Compare May 30, 2023 14:54

mergify bot added ci-passed and removed ci-passed labels May 30, 2023

chyezh requested review from jaime0815 and yiwangdr May 31, 2023 01:54

yiwangdr approved these changes May 31, 2023

View reviewed changes

sre-ci-robot assigned czs007 May 31, 2023

czs007 reviewed Jun 6, 2023

View reviewed changes

pkg/util/paramtable/service_param.go Outdated Show resolved Hide resolved

czs007 reviewed Jun 6, 2023

View reviewed changes

pkg/mq/msgstream/mqwrapper/nmq/nmq_consumer.go Show resolved Hide resolved

add nats mq wrappers

e9f22af

Signed-off-by: yiwangdr <yiwangdr@gmail.com> bug fixup, configurable natsmq, add unittest, pass e2e. Signed-off-by: chyezh <ye.zhen@zilliz.com> move natsmq to pkg project Signed-off-by: chyezh <ye.zhen@zilliz.com>

chyezh force-pushed the feature_enable_natsmq branch from c6b4e89 to e9f22af Compare June 6, 2023 12:34

mergify bot removed the ci-passed label Jun 6, 2023

mergify bot added the ci-passed label Jun 6, 2023

sre-ci-robot added the lgtm label Jun 7, 2023

sre-ci-robot added the approved label Jun 7, 2023

sre-ci-robot merged commit f97127a into milvus-io:master Jun 7, 2023

chyezh deleted the feature_enable_natsmq branch June 7, 2023 02:03

[Feature] Enable Nats-server as a optional MQ for Milvus #24445

[Feature] Enable Nats-server as a optional MQ for Milvus #24445

Conversation

chyezh commented May 26, 2023

mergify bot commented May 26, 2023

mergify bot commented May 26, 2023

mergify bot commented May 26, 2023

mergify bot commented May 26, 2023

codecov bot commented May 26, 2023 • edited Loading

Codecov Report

mergify bot commented May 27, 2023

mergify bot commented May 27, 2023

chyezh commented May 27, 2023

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

chyezh commented May 30, 2023

mergify bot commented May 30, 2023

chyezh commented May 30, 2023

yiwangdr commented May 30, 2023

chyezh commented May 31, 2023

mergify bot commented Jun 6, 2023

chyezh commented Jun 6, 2023

mergify bot commented Jun 6, 2023

chyezh commented Jun 6, 2023

czs007 commented Jun 7, 2023

sre-ci-robot commented Jun 7, 2023

codecov bot commented May 26, 2023 •

edited

Loading