Re: [PR] Add `has-schema-role` flag to switch schema property should activate or not [skywalking-banyandb]
hanahmily commented on code in PR #1007:
URL:
https://github.com/apache/skywalking-banyandb/pull/1007#discussion_r2923863062
##
banyand/metadata/service/server.go:
##
@@ -82,6 +82,7 @@ type server struct {
listenPeerURL []string
quotaBackendBytes run.Bytes
embeddedbool
+ hasSchemaRole bool
Review Comment:
You did not bind this var to a flag. The name should be "hasMetaRole".
BanyanDB does not have a role named "schema"
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add `has-schema-role` flag to switch schema property should activate or not [skywalking-banyandb]
mrproliu commented on code in PR #1007:
URL:
https://github.com/apache/skywalking-banyandb/pull/1007#discussion_r2922880155
##
pkg/test/setup/setup.go:
##
@@ -609,13 +609,23 @@ func CMD(flags ...string) func() {
}
}
+func hasFlag(flags []string, target string) bool {
+ for _, f := range flags {
+ if f == target {
+ return true
+ }
+ }
+ return false
+}
+
func startDataNode(config *ClusterConfig, dataDir string, flags ...string)
(string, string, func()) {
if config == nil {
config = defaultClusterConfig
}
isPropertyMode := config.SchemaRegistry.Mode == ModeProperty
+ runSchemaServer := isPropertyMode && !hasFlag(flags,
"--has-schema-role=false")
portCount := 2
- if isPropertyMode {
+ if runSchemaServer {
portCount = 3
Review Comment:
Update the flag verify logical.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add `has-schema-role` flag to switch schema property should activate or not [skywalking-banyandb]
mrproliu commented on code in PR #1007:
URL:
https://github.com/apache/skywalking-banyandb/pull/1007#discussion_r2922876954
##
test/integration/distributed/lifecycle/property/suite_test.go:
##
@@ -102,19 +100,34 @@ func init() {
}
}
-func diffGroups(left, right []string) []string {
- rightSet := make(map[string]struct{}, len(right))
- for _, groupName := range right {
- rightSet[groupName] = struct{}{}
- }
- result := make([]string, 0)
- for _, groupName := range left {
- if _, exists := rightSet[groupName]; exists {
- continue
+func verifyClusterNodeRoles(liaisonAddr string) {
+ conn, connErr := grpchelper.Conn(liaisonAddr, 10*time.Second,
+ grpclib.WithTransportCredentials(insecure.NewCredentials()))
+ Expect(connErr).NotTo(HaveOccurred())
+ defer func() { _ = conn.Close() }()
+ clusterClient := databasev1.NewClusterStateServiceClient(conn)
+ state, stateErr := clusterClient.GetClusterState(
+ context.Background(), &databasev1.GetClusterStateRequest{})
+ Expect(stateErr).NotTo(HaveOccurred())
+ tire2 := state.GetRouteTables()["tire2"]
+ Expect(tire2).NotTo(BeNil(), "tire2 route table not found")
+ for _, node := range tire2.GetRegistered() {
+ labels := node.GetLabels()
+ hasMetaRole := false
+ for _, role := range node.GetRoles() {
+ if role == databasev1.Role_ROLE_META {
+ hasMetaRole = true
+ break
+ }
+ }
+ if labels["type"] == "hot" {
+ Expect(hasMetaRole).To(BeTrue(),
+ fmt.Sprintf("hot node %s should have
ROLE_META", node.GetMetadata().GetName()))
+ } else if labels["type"] == "warm" {
+ Expect(hasMetaRole).To(BeFalse(),
+ fmt.Sprintf("warm node %s should NOT have
ROLE_META", node.GetMetadata().GetName()))
}
Review Comment:
Added the node count check.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add `has-schema-role` flag to switch schema property should activate or not [skywalking-banyandb]
mrproliu commented on code in PR #1007:
URL:
https://github.com/apache/skywalking-banyandb/pull/1007#discussion_r2922875482
##
test/integration/distributed/lifecycle/property/suite_test.go:
##
@@ -102,19 +100,34 @@ func init() {
}
}
-func diffGroups(left, right []string) []string {
- rightSet := make(map[string]struct{}, len(right))
- for _, groupName := range right {
- rightSet[groupName] = struct{}{}
- }
- result := make([]string, 0)
- for _, groupName := range left {
- if _, exists := rightSet[groupName]; exists {
- continue
+func verifyClusterNodeRoles(liaisonAddr string) {
+ conn, connErr := grpchelper.Conn(liaisonAddr, 10*time.Second,
+ grpclib.WithTransportCredentials(insecure.NewCredentials()))
+ Expect(connErr).NotTo(HaveOccurred())
+ defer func() { _ = conn.Close() }()
+ clusterClient := databasev1.NewClusterStateServiceClient(conn)
+ state, stateErr := clusterClient.GetClusterState(
+ context.Background(), &databasev1.GetClusterStateRequest{})
+ Expect(stateErr).NotTo(HaveOccurred())
Review Comment:
Wrapped with `Eventually`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Add `has-schema-role` flag to switch schema property should activate or not [skywalking-banyandb]
codecov-commenter commented on PR #1007: URL: https://github.com/apache/skywalking-banyandb/pull/1007#issuecomment-4044618672 ## [Codecov](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1007?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report :white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 30.07%. Comparing base ([`3530dd9`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/3530dd9b5cefaa4fef60eec91ecbfbdfcaf243c5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7e60e58`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/7e60e589d1e22a0c898c8dbc49fec56dc4de73ba?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)). :warning: Report is 154 commits behind head on main. > :exclamation: There is a different number of reports uploaded between BASE (3530dd9) and HEAD (7e60e58). Click for more details. > > HEAD has 1 upload less than BASE > >| Flag | BASE (3530dd9) | HEAD (7e60e58) | >|--|--|--| >||1|0| > Additional details and impacted files ```diff @@ Coverage Diff @@ ## main#1007 +/- ## === - Coverage 45.97% 30.07% -15.91% === Files 328 123 -205 Lines 5550512850-42655 === - Hits25520 3864-21656 + Misses 27909 8614-19295 + Partials 2076 372 -1704 ``` | [Flag](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1007/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [pkg](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1007/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `30.07% <ø> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1007?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :rocket: New features to boost your workflow: - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems. - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Add `has-schema-role` flag to switch schema property should activate or not [skywalking-banyandb]
Copilot commented on code in PR #1007:
URL:
https://github.com/apache/skywalking-banyandb/pull/1007#discussion_r2922770906
##
pkg/test/setup/setup.go:
##
@@ -609,13 +609,23 @@ func CMD(flags ...string) func() {
}
}
+func hasFlag(flags []string, target string) bool {
+ for _, f := range flags {
+ if f == target {
+ return true
+ }
+ }
+ return false
+}
+
func startDataNode(config *ClusterConfig, dataDir string, flags ...string)
(string, string, func()) {
if config == nil {
config = defaultClusterConfig
}
isPropertyMode := config.SchemaRegistry.Mode == ModeProperty
+ runSchemaServer := isPropertyMode && !hasFlag(flags,
"--has-schema-role=false")
portCount := 2
- if isPropertyMode {
+ if runSchemaServer {
portCount = 3
Review Comment:
`hasFlag` only matches an exact string (e.g. `--has-schema-role=false`). If
callers pass the bool flag as separate args (`--has-schema-role`, `false`) or
use `--has-schema-role=false` with extra whitespace, `runSchemaServer` will be
computed incorrectly, leading to allocating the wrong number of ports and
potentially adding a schema server address even though the node won’t start
one. Consider parsing the flag more robustly (handle both `--k=v` and `--k v`,
or use the flag parser) before deciding `portCount`/`runSchemaServer`.
##
test/integration/distributed/lifecycle/property/suite_test.go:
##
@@ -102,19 +100,34 @@ func init() {
}
}
-func diffGroups(left, right []string) []string {
- rightSet := make(map[string]struct{}, len(right))
- for _, groupName := range right {
- rightSet[groupName] = struct{}{}
- }
- result := make([]string, 0)
- for _, groupName := range left {
- if _, exists := rightSet[groupName]; exists {
- continue
+func verifyClusterNodeRoles(liaisonAddr string) {
+ conn, connErr := grpchelper.Conn(liaisonAddr, 10*time.Second,
+ grpclib.WithTransportCredentials(insecure.NewCredentials()))
+ Expect(connErr).NotTo(HaveOccurred())
+ defer func() { _ = conn.Close() }()
+ clusterClient := databasev1.NewClusterStateServiceClient(conn)
+ state, stateErr := clusterClient.GetClusterState(
+ context.Background(), &databasev1.GetClusterStateRequest{})
+ Expect(stateErr).NotTo(HaveOccurred())
+ tire2 := state.GetRouteTables()["tire2"]
+ Expect(tire2).NotTo(BeNil(), "tire2 route table not found")
+ for _, node := range tire2.GetRegistered() {
+ labels := node.GetLabels()
+ hasMetaRole := false
+ for _, role := range node.GetRoles() {
+ if role == databasev1.Role_ROLE_META {
+ hasMetaRole = true
+ break
+ }
+ }
+ if labels["type"] == "hot" {
+ Expect(hasMetaRole).To(BeTrue(),
+ fmt.Sprintf("hot node %s should have
ROLE_META", node.GetMetadata().GetName()))
+ } else if labels["type"] == "warm" {
+ Expect(hasMetaRole).To(BeFalse(),
+ fmt.Sprintf("warm node %s should NOT have
ROLE_META", node.GetMetadata().GetName()))
}
Review Comment:
This loop only asserts role expectations *if* it encounters a node with
label `type=hot`/`type=warm`. If the warm node never registers (or labels are
missing), the test can still pass. Track whether at least one hot and one warm
node were observed and `Expect` both to be true after iterating.
##
test/integration/distributed/lifecycle/property/suite_test.go:
##
@@ -102,19 +100,34 @@ func init() {
}
}
-func diffGroups(left, right []string) []string {
- rightSet := make(map[string]struct{}, len(right))
- for _, groupName := range right {
- rightSet[groupName] = struct{}{}
- }
- result := make([]string, 0)
- for _, groupName := range left {
- if _, exists := rightSet[groupName]; exists {
- continue
+func verifyClusterNodeRoles(liaisonAddr string) {
+ conn, connErr := grpchelper.Conn(liaisonAddr, 10*time.Second,
+ grpclib.WithTransportCredentials(insecure.NewCredentials()))
+ Expect(connErr).NotTo(HaveOccurred())
+ defer func() { _ = conn.Close() }()
+ clusterClient := databasev1.NewClusterStateServiceClient(conn)
+ state, stateErr := clusterClient.GetClusterState(
+ context.Background(), &databasev1.GetClusterStateRequest{})
+ Expect(stateErr).NotTo(HaveOccurred())
Review Comment:
`verifyClusterNodeRoles` makes a single `GetClusterState` call with
`context.Background()`. Because cluster
