[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #239: Unify TagFilter for Streaming scenario

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

hanahmily commented on code in PR #239:
URL: https://github.com/apache/skywalking-banyandb/pull/239#discussion_r1072158568


##########
banyand/measure/measure_write.go:
##########
@@ -121,6 +122,9 @@ func (s *measure) write(md *commonv1.Metadata, shardID common.ShardID, entity []
 		_ = wp.Close()
 		return err
 	}
+	if isVirtualMeasure {

Review Comment:
   You don't have to add this flag. The `measure` stores topn's results don't refer to any `indexRules`. It won't generate any indices. 
   
   



##########
banyand/measure/measure_write.go:
##########
@@ -39,17 +39,18 @@ import (
 
 var errMalformedElement = errors.New("element is malformed")
 
-func (s *measure) write(md *commonv1.Metadata, shardID common.ShardID, entity []byte, entityValues tsdb.EntityValues, value *measurev1.DataPointValue) error {
+func (s *measure) write(sm *databasev1.Measure, shardID common.ShardID, entity []byte, entityValues tsdb.EntityValues,

Review Comment:
   Injecting a faked measure is a terrible design that will mess up the writing process. 
   You had better build and register a new `measure` to write topN's result. 



##########
banyand/measure/measure_write.go:
##########
@@ -60,7 +61,7 @@ func (s *measure) write(md *commonv1.Metadata, shardID common.ShardID, entity []
 	if err != nil {
 		return err
 	}
-	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	ctx, cancel := context.WithTimeout(context.TODO(), 5*time.Second)

Review Comment:
   Why do you change it to `TODO`? This context is the top-level, `Background` is better here. 



-- 
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