hanahmily commented on code in PR #680:
URL:
https://github.com/apache/skywalking-banyandb/pull/680#discussion_r2162836856
##########
Makefile:
##########
@@ -206,4 +206,4 @@ release-push-candidate: ## Push release candidate
.PHONY: test test-race test-coverage test-ci
.PHONY: license-check license-fix license-dep
.PHONY: release release-binary release-source release-sign release-assembly
-.PHONY: vendor-update
Review Comment:
Do not change this file
##########
banyand/protector/protector.go:
##########
@@ -227,3 +228,36 @@ func (m *memory) Serve() run.StopNotify {
}()
return m.closed
}
+
+// GetThreshold returns the threshold for large file detection (1% of page
cache).
+func (m *memory) GetThreshold() int64 {
+ // Try reading cgroup memory limit
+ cgLimit, err := cgroups.MemoryLimit()
+ if err != nil {
+ m.l.Warn().Err(err).Msg("failed to get memory limit from
cgroups, using default threshold")
+ // Fallback default threshold of 64MB
+ return 64 << 20
+ }
+
+ // Determine effective memory to use based on flags
+ var totalMemory int64
+ if m.allowedBytes > 0 {
+ totalMemory = cgLimit - int64(m.allowedBytes)
+ } else {
+ totalMemory = cgLimit * int64(m.allowedPercent) / 100
+ }
+
+ // Compute 1% of that memory as page cache threshold
+ threshold := totalMemory / 100
+ const minThreshold = 10 << 20 // 10MB
+ if threshold < minThreshold {
+ threshold = minThreshold
+ }
+ return threshold
+}
+
+// ShouldApplyFadvis implements the fs.ThresholdProvider interface.
+// It checks if the file size exceeds the threshold for large file detection.
+func (m *memory) ShouldApplyFadvis(fileSize int64) bool {
Review Comment:
```suggestion
func (m *memory) ShouldCache(fileSize int64) bool {
```
You should use a unique term consistently. In the current implementation,
"ShouldCache" is more appropriate.
##########
banyand/measure/merger_test.go:
##########
@@ -28,9 +29,28 @@ import (
"github.com/apache/skywalking-banyandb/pkg/fs"
pbv1 "github.com/apache/skywalking-banyandb/pkg/pb/v1"
+ "github.com/apache/skywalking-banyandb/pkg/run"
"github.com/apache/skywalking-banyandb/pkg/test"
)
+// nopProtector is a dummy Protector implementation for tests.
Review Comment:
Could you move it to the protector package? The stream and measure
components could share it.
##########
banyand/measure/tstable.go:
##########
@@ -47,13 +48,19 @@ const (
func newTSTable(fileSystem fs.FileSystem, rootPath string, p common.Position,
l *logger.Logger, _ timestamp.TimeRange, option option, m any,
) (*tsTable, error) {
+ if option.protector == nil {
+ logger.GetLogger("measure").
+ Panic().
+ Msg("protector is nil in newTSTable, using default
protector")
Review Comment:
```suggestion
Msg("protector can not be nil")
```
##########
banyand/measure/metadata.go:
##########
@@ -352,11 +352,23 @@ type supplier struct {
}
func newSupplier(path string, svc *service, sr *schemaRepo, nodeLabels
map[string]string) *supplier {
+ if svc.pm == nil {
+ svc.l.Error().Msg("CRITICAL: svc.pm is nil in newSupplier")
+ }
+ opt := svc.option
+ opt.protector = svc.pm
+
+ if opt.protector == nil {
+ svc.l.Error().Msg("CRITICAL: opt.protector is still nil after
assignment")
Review Comment:
Use Panic instead of Error
##########
banyand/measure/metadata.go:
##########
@@ -352,11 +352,23 @@ type supplier struct {
}
func newSupplier(path string, svc *service, sr *schemaRepo, nodeLabels
map[string]string) *supplier {
+ if svc.pm == nil {
+ svc.l.Error().Msg("CRITICAL: svc.pm is nil in newSupplier")
+ }
+ opt := svc.option
+ opt.protector = svc.pm
+
+ if opt.protector == nil {
+ svc.l.Error().Msg("CRITICAL: opt.protector is still nil after
assignment")
+ } else {
+ svc.l.Info().Msg("opt.protector successfully set in
newSupplier")
Review Comment:
```suggestion
```
Do not print a necessary message.
##########
banyand/internal/test/mock_memory_protector.go:
##########
@@ -49,6 +49,16 @@ func (f *MockMemoryProtector) AcquireResource(_
context.Context, _ uint64) error
return f.acquireErr
}
+// ShouldApplyFadvis always returns false for testing.
+func (f *MockMemoryProtector) ShouldApplyFadvis(_ int64) bool {
+ return false
+}
Review Comment:
```suggestion
```
##########
.gitignore:
##########
@@ -63,4 +63,4 @@ gomock_reflect*
.stignore
# profile result
-*.prof
Review Comment:
Do not change this file
##########
banyand/measure/service.go:
##########
@@ -134,6 +134,11 @@ func (s *service) Role() databasev1.Role {
func (s *service) PreRun(ctx context.Context) error {
s.l = logger.GetLogger(s.Name())
+ if s.pm == nil {
+ s.l.Error().Msg("CRITICAL: memory protector is nil in PreRun")
+ return errors.New("memory protector is required but was nil")
+ }
+ s.l.Info().Msg("memory protector is initialized in PreRun")
Review Comment:
```suggestion
```
--
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-unsubscr...@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
