[GitHub] [skywalking-banyandb] hanahmily commented on a change in pull request #65: WIP: metadata reload

[GitBox <gi...@apache.org>]

hanahmily commented on a change in pull request #65:
URL: https://github.com/apache/skywalking-banyandb/pull/65#discussion_r781160261



##########
File path: api/proto/banyandb/measure/v1/rpc.pb.go
##########
@@ -18,7 +18,7 @@
 // Code generated by protoc-gen-go. DO NOT EDIT.
 // versions:
 // 	protoc-gen-go v1.27.1
-// 	protoc        v3.18.1

Review comment:
       why is the version of `protoc` absent?

##########
File path: api/proto/banyandb/common/v1/common.proto
##########
@@ -37,6 +37,20 @@ message Metadata {
     // name of the entity
     string name = 2;
     uint32 id = 3;
+    // list of objects depended by this object.
+    // This field is maintained by the metadata module and only for internal usage.
+    // The referent should be reconciled after the current object is deleted.
+    repeated OwnerReference owner_references = 4;
+}
+
+// OwnerReference contains enough information to let you identify an owning object.
+// The group is omitted since an owning object must be in the same group as the dependent.
+// The kind of the owning object depends implicitly on the kind of the dependent.
+// Currently,
+// IndexRule --(owner_references)--> IndexRuleBinding --(explicitly with subject)--> stream/measure
+message OwnerReference {

Review comment:
       We don't need this since indexRuleBindings have all relationships between indexrules and stream/measue.
   
   The API has adopted the many-to-many relationship, but the owner object limits it to a one-to-many type. It conflicts the original design of this API

##########
File path: banyand/metadata/schema/etcd.go
##########
@@ -282,15 +314,104 @@ func (e *etcdSchemaRegistry) UpdateIndexRuleBinding(ctx context.Context, indexRu
 	if err != nil {
 		return errors.Wrap(err, indexRuleBinding.GetMetadata().GetGroup())
 	}
-	return e.update(ctx, g, formatIndexRuleBindingKey(indexRuleBinding.GetMetadata()), indexRuleBinding)
+
+	irs := make(map[string]*databasev1.IndexRule)
+	// ensure the existence of referencing indexRule(s)
+	for _, irName := range indexRuleBinding.GetRules() {
+		ir, innerErr := e.GetIndexRule(ctx, &commonv1.Metadata{
+			Name:  irName,
+			Group: g.GetName(),
+		})
+		if innerErr != nil {
+			return errors.Wrap(innerErr, "index rule does not exist")
+		}
+
+		irs[irName] = ir
+	}
+
+	// sort rules in ASC order
+	sort.Strings(indexRuleBinding.GetRules())
+
+	// try to find the existing instance
+	// save a copy of rule refs if exists
+	var oldRuleRefs []string
+	old, err := e.GetIndexRuleBinding(ctx, &commonv1.Metadata{
+		Name: indexRuleBinding.GetMetadata().GetName(),
+	})
+
+	if err == nil && old != nil {
+		oldRuleRefs = old.GetRules()
+	}
+
+	err = e.update(ctx, g, &resource{

Review comment:
       Why do you tend to update indexrule when updating indexrulebinding? They are both independent objects, don't have to sync with each other.

##########
File path: banyand/metadata/schema/etcd.go
##########
@@ -402,15 +534,49 @@ func (e *etcdSchemaRegistry) get(ctx context.Context, key string, message proto.
 	return nil
 }
 
-func (e *etcdSchemaRegistry) update(ctx context.Context, group *commonv1.Group, key string, message proto.Message) error {
-	val, err := proto.Marshal(message)
+func (e *etcdSchemaRegistry) update(ctx context.Context, group *commonv1.Group, res *resource, notification bool) error {
+	val, err := proto.Marshal(res.Message)
 	if err != nil {
 		return err
 	}
-	_, err = e.kv.Put(ctx, key, string(val))
+	_, err = e.kv.Put(ctx, res.Key(), string(val))
 	if err != nil {
 		return err
 	}
+
+	if notification {
+		switch res.typ {

Review comment:
       From my perspective, all updating event handling processes should be identical. 

##########
File path: banyand/metadata/schema/etcd.go
##########
@@ -130,6 +140,7 @@ func (e *etcdSchemaRegistry) DeleteGroup(ctx context.Context, group string) (boo
 	}
 	keyPrefix := GroupsKeyPrefix + g.GetName() + "/"
 	_, err = e.kv.Delete(ctx, keyPrefix, clientv3.WithRange(incrementLastByte(keyPrefix)))
+	// TODO: which resources are deleted?

Review comment:
       What's the TODO means? Do not the resources have the same prefix?

##########
File path: banyand/metadata/schema/etcd.go
##########
@@ -402,15 +534,49 @@ func (e *etcdSchemaRegistry) get(ctx context.Context, key string, message proto.
 	return nil
 }
 
-func (e *etcdSchemaRegistry) update(ctx context.Context, group *commonv1.Group, key string, message proto.Message) error {
-	val, err := proto.Marshal(message)
+func (e *etcdSchemaRegistry) update(ctx context.Context, group *commonv1.Group, res *resource, notification bool) error {
+	val, err := proto.Marshal(res.Message)
 	if err != nil {
 		return err
 	}
-	_, err = e.kv.Put(ctx, key, string(val))
+	_, err = e.kv.Put(ctx, res.Key(), string(val))
 	if err != nil {
 		return err
 	}
+
+	if notification {
+		switch res.typ {
+		case ResourceStream:
+			e.onEntityUpdate(commonv1.Catalog_CATALOG_STREAM, res.GetMetadata())
+		case ResourceMeasure:
+			e.onEntityUpdate(commonv1.Catalog_CATALOG_MEASURE, res.GetMetadata())
+		case ResourceIndexRuleBinding:
+			rb := res.Message.(*databasev1.IndexRuleBinding)
+			e.onEntityUpdate(rb.GetSubject().GetCatalog(), &commonv1.Metadata{
+				Name:  rb.GetSubject().GetName(),
+				Group: group.GetName(),
+			})
+		case ResourceIndexRule:
+			for _, ownerRef := range res.GetMetadata().GetOwnerReferences() {
+				rb, innerErr := e.GetIndexRuleBinding(ctx, &commonv1.Metadata{
+					Name:  ownerRef.GetName(),
+					Group: group.GetName(),
+				})
+
+				if innerErr != nil {
+					// TODO: possible inconsistent state?

Review comment:
       The handler should be idempotent. The notifier has to retry the handler till it returns no error. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org