ronantakizawa/codereview-bench huggingface dataset:
Repository: influxdata/influxdb
File: tsdb/store.go (Go)

Code before:
}

// Shard returns a shard by id.
func (s *Store) Shard(id uint64) *Shard {
	s.mu.RLock()
	defer s.mu.RUnlock()
	sh, ok := s.shards[id]
	if !ok {
		return nil
	}
	return sh
}

type ErrPreviousShardFail struct {
	error
}

func (e ErrPreviousShardFail) Unwrap() error {
	return e.error
}

func (e ErrPreviousShardFail) Is(err error) bool {
	_, sOk := err.(ErrPreviousShardFail)
	_, pOk := err.(*ErrPreviousShardFail)
	return sOk || pOk
}

func (e ErrPreviousShardFail) Error() string {
	return e.error.Error()
}

func (s *Store) ReopenShard(ctx context.Context, shardID uint64, force bool) error {
	sh := s.Shard(shardID)
	if sh == nil {
		return ErrShardNotFound
	}

	s.mu.Lock()
	defer s.mu.Unlock()

	loader := s.newShardLoader(shardID, "", "", true, withExistingShard(sh), withForceLoad(force))
	res := loader.Load(ctx)
	s.registerShard(res)

	return res.err
}

func (s *Store) SetShardOpenErrorForTest(shardID uint64, err error) {
	s.badShards.setShardOpenError(shardID, err)
}


Review comment (bug):
There's a small theoretical race condition here, because `s.badShards.shardErrors` could be modified between `GetBadShardList` and clearing it. In practice, this won't happen since the only time you really see shards getting added to the bad shard list is on startup.

Code after:
	return s.sfiles[database]
}

// Shard returns a shard by id.
func (s *Store) Shard(id uint64) *Shard {
	s.mu.RLock()
	defer s.mu.RUnlock()
	sh, ok := s.shards[id]
	if !ok {
		return nil
	}
	return sh
}

// ClearBadShardList will remove all shards from the badShards cache
// this will allow for lazy loading of bad shards if/when they are no
// longer in a "bad" state. This method will return any shards that
// were removed from the cache.
func (s *Store) ClearBadShardList() map[uint64]error {
	badShards := s.GetBadShardList()

	s.mu.Lock()
	defer s.mu.Unlock()
	clear(s.badShards.shardErrors)

	return badShards
}

// GetBadShardList is exposed as a method for test purposes
func (s *Store) GetBadShardList() map[uint64]error {
	s.badShards.mu.Lock()
	defer s.badShards.mu.Unlock()

	if s.badShards.shardErrors == nil {
		s.Logger.Warn("badShards was nil")
		s.badShards.shardErrors = make(map[uint64]error)
	}

	shardList := maps.Clone(s.badShards.shardErrors)

	return shardList
}

type ErrPreviousShardFail struct {
	error
}

func (e ErrPreviousShardFail) Unwrap() error {
	return e.error
}
Repository: influxdata/influxdb
File: tsdb/engine/tsm1/file_store.go (Go)

Code before:
}

type purger struct {
	files   gensyncmap.Map[string, TSMFile]
	mu      sync.Mutex
	running bool

	logger *zap.Logger
}

func (p *purger) add(files []TSMFile) {
	if len(files) == 0 {
		return
	}

	p.mu.Lock()
	defer p.mu.Unlock()

	var fileNames []string
	for _, f := range files {
		fileName := f.Path()
		fileNames = append(fileNames, fileName)
		p.files.Store(fileName, f)
	}

	p.purge(fileNames)
}

// purge starts a goroutine to purge files from disk if one isn't already running.
// Must be called with p.mu held.
func (p *purger) purge(fileNames []string) {
	logger, logEndOp := logger.NewOperation(p.logger, "Purge held files", "filestore_purger")

	logger.Info("added", zap.Int("count", len(fileNames)))
	logger.Debug("purging", zap.Strings("files", fileNames))

	if p.running {
		logger.Info("already running, files added to previous operation")
		logEndOp()
		return
	}

	p.running = true

	go func() {
		var purgeCount int
		var failCount int
		defer func() {
			logger.Info("removed", zap.Int("files", purgeCount))
			if failCount > 0 {
				logger.Warn("failed to remove", zap.Int("files", failCount))

Review comment (suggestion):
The mutex protects the entire add() operation including the Store() calls to sync.Map. While sync.Map is designed for concurrent access without external locking, holding the mutex here means concurrent add() calls will serialize. Consider restructuring to only hold the mutex when checking/setting the running flag in purge(), allowing the sync.Map Store operations (lines 1615-1619) to happen concurrently. This would maximize the benefit of using sync.Map for lock-free concurrent access.

Code after:
}

// purger manages asynchronous deletion of TSM files that have been
// replaced by compaction, but are temporarily held open by queries
type purger struct {
	files   gensyncmap.Map[string, TSMFile]
	mu      sync.Mutex
	running bool

	logger *zap.Logger
}

func (p *purger) add(files []TSMFile) {
	if len(files) == 0 {
		return
	}

	var fileNames []string
	for _, f := range files {
		fileName := f.Path()
		fileNames = append(fileNames, fileName)
		p.files.Store(fileName, f)
	}

	p.purge(fileNames)
}

// purge starts a goroutine to purge files from disk if one isn't already running.
func (p *purger) purge(fileNames []string) {
	logger, logEndOp := logger.NewOperation(p.logger, "Purge held files", "filestore_purger")

	logger.Info("added", zap.Int("count", len(fileNames)))
	logger.Debug("purging", zap.Strings("files", fileNames))

	p.mu.Lock()
	defer p.mu.Unlock()
	if p.running {
		logger.Info("already running, files added to previous operation")
		logEndOp()
		return
	}
	p.running = true

	go func() {
		var purgeCount int
		var failCount int
		defer func() {
			logger.Info("removed", zap.Int("files", purgeCount))
			if failCount > 0 {
				logger.Warn("failed to remove", zap.Int("files", failCount))
			}
Repository: influxdata/influxdb
File: tsdb/engine/tsm1/file_store.go (Go)

Code before:
						failCount++
					} else if err := v.Remove(); err != nil {
						logger.Error("remove file failed", zap.String("file", k), zap.Error(err))
						failCount++
					} else {
						logger.Debug("successfully removed", zap.String("file", k))
						purgeCount++
					}
					// Remove the file regardless of success or failure.
					// Do not retry files which could not be closed or removed.
					p.files.Delete(k)
				}
				// InUse files are left to be tried later.
				return true
			})

			time.Sleep(time.Second)
		}
	}()
}

func (p *purger) hasFiles() bool {
	p.mu.Lock()
	defer p.mu.Unlock()

	if p.files.Len() == 0 {
		p.running = false
		return false
	}
	return true
}

type tsmReaders []TSMFile

func (a tsmReaders) Len() int           { return len(a) }
func (a tsmReaders) Less(i, j int) bool { return a[i].Path() < a[j].Path() }
func (a tsmReaders) Swap(i, j int)      { a[i], a[j] = a[j], a[i] }


Review comment (performance):
The Len() method iterates through the entire sync.Map using Range(), which can be expensive if there are many files. This is called on every iteration of the purge loop (line 1654). Consider tracking the count separately using an atomic counter that's incremented on Store() and decremented on Delete(), or cache the length check result to avoid repeated full map scans.
```suggestion
	// Avoid using Len(), which iterates the entire underlying map.
	// Instead, check for emptiness by stopping Range() after the first entry.
	has := false
	p.files.Range(func(k string, v TSMFile) bool {
		has = true
		return false // stop iteration after finding the first file
	})

	if !has {
```

Code after:
					} else if err := v.Remove(); err != nil {
						logger.Error("remove file failed", zap.String("file", k), zap.Error(err))
						failCount++
					} else {
						logger.Debug("successfully removed", zap.String("file", k))
						purgeCount++
					}
					// Remove the file regardless of success or failure.
					// Do not retry files which could not be closed or removed.
					// This is because they have already been closed, or there
					// is an operating system problem that is unlikely to
					// resolve by itself.
					p.files.Delete(k)
				}
				// InUse files are left to be tried later.
				return true
			})

			time.Sleep(time.Second)
		}
	}()
}

func (p *purger) hasFiles() bool {
	p.mu.Lock()
	defer p.mu.Unlock()

	has := false

	// Avoid calling Len() which iterates over the whole map.
	p.files.Range(func(k string, v TSMFile) bool {
		has = true
		return false // stop iteration after finding the first file
	})
	p.running = has
	return has
}

type tsmReaders []TSMFile

func (a tsmReaders) Len() int           { return len(a) }
func (a tsmReaders) Less(i, j int) bool { return a[i].Path() < a[j].Path() }
func (a tsmReaders) Swap(i, j int)      { a[i], a[j] = a[j], a[i] }
Repository: influxdata/influxdb
File: tsdb/engine/tsm1/file_store.go (Go)

Code before:
	// Append the first matching block
	if len(c.current) == 0 {
		c.current = make([]*location, 1)
	} else {
		c.current = c.current[:1]
	}
	c.current[0] = c.seeks[c.pos]

	// If we have ovelapping blocks, append all their values so we can dedup
	for i := c.pos; i >= 0; i-- {
		if c.seeks[i].read() {
			continue
		}
		c.current = append(c.current, c.seeks[i])
	}
}

type purger struct {
	files   gensyncmap.Map[string, TSMFile]
	mu      sync.Mutex
	running bool

	logger *zap.Logger
}

func (p *purger) add(files []TSMFile) {
	if len(files) == 0 {
		return
	}

	p.mu.Lock()
	defer p.mu.Unlock()

	var fileNames []string
	for _, f := range files {
		fileName := f.Path()
		fileNames = append(fileNames, fileName)
		p.files.Store(fileName, f)
	}

	p.purge(fileNames)
}

// purge starts a goroutine to purge files from disk if one isn't already running.
// Must be called with p.mu held.
func (p *purger) purge(fileNames []string) {
	logger, logEndOp := logger.NewOperation(p.logger, "Purge held files", "filestore_purger")

	logger.Info("added", zap.Int("count", len(fileNames)))
	logger.Debug("purging", zap.Strings("files", fileNames))


Review comment (suggestion):
Consider adding a doc comment to the purger type to explain its purpose. For example: "purger manages asynchronous deletion of TSM files that are no longer in use by queries but couldn't be deleted immediately during compaction."

Code after:
	// Append the first matching block
	if len(c.current) == 0 {
		c.current = make([]*location, 1)
	} else {
		c.current = c.current[:1]
	}
	c.current[0] = c.seeks[c.pos]

	// If we have ovelapping blocks, append all their values so we can dedup
	for i := c.pos; i >= 0; i-- {
		if c.seeks[i].read() {
			continue
		}
		c.current = append(c.current, c.seeks[i])
	}
}

// purger manages asynchronous deletion of TSM files that have been
// replaced by compaction, but are temporarily held open by queries
type purger struct {
	files   gensyncmap.Map[string, TSMFile]
	mu      sync.Mutex
	running bool

	logger *zap.Logger
}

func (p *purger) add(files []TSMFile) {
	if len(files) == 0 {
		return
	}

	var fileNames []string
	for _, f := range files {
		fileName := f.Path()
		fileNames = append(fileNames, fileName)
		p.files.Store(fileName, f)
	}

	p.purge(fileNames)
}

// purge starts a goroutine to purge files from disk if one isn't already running.
func (p *purger) purge(fileNames []string) {
	logger, logEndOp := logger.NewOperation(p.logger, "Purge held files", "filestore_purger")

	logger.Info("added", zap.Int("count", len(fileNames)))
	logger.Debug("purging", zap.Strings("files", fileNames))

	p.mu.Lock()
	defer p.mu.Unlock()
Repository: influxdata/influxdb
File: tsdb/engine/tsm1/file_store.go (Go)

Code before:
						logger.Debug("successfully removed", zap.String("file", k))
						purgeCount++
					}
					// Remove the file regardless of success or failure.
					// Do not retry files which could not be closed or removed.
					// This is because they have already been closed, or there
					// is an operating system problem that is unlikely to
					// resolve by itself.
					p.files.Delete(k)
				}
				// InUse files are left to be tried later.
				return true
			})

			time.Sleep(time.Second)
		}
	}()
}

func (p *purger) hasFiles() bool {
	p.mu.Lock()
	defer p.mu.Unlock()

	has := false

	/// Avoid calling Len() which iterates over the whole map.
	p.files.Range(func(k string, v TSMFile) bool {
		has = true
		return false // stop iteration after finding the first file
	})
	p.running = has
	return has
}

type tsmReaders []TSMFile

func (a tsmReaders) Len() int           { return len(a) }
func (a tsmReaders) Less(i, j int) bool { return a[i].Path() < a[j].Path() }
func (a tsmReaders) Swap(i, j int)      { a[i], a[j] = a[j], a[i] }


Review comment (suggestion):
The comment has three slashes instead of two. Change "///" to "//" to follow standard Go comment conventions.
```suggestion
	// Avoid calling Len() which iterates over the whole map.
```

Code after:
						logger.Debug("successfully removed", zap.String("file", k))
						purgeCount++
					}
					// Remove the file regardless of success or failure.
					// Do not retry files which could not be closed or removed.
					// This is because they have already been closed, or there
					// is an operating system problem that is unlikely to
					// resolve by itself.
					p.files.Delete(k)
				}
				// InUse files are left to be tried later.
				return true
			})

			time.Sleep(time.Second)
		}
	}()
}

func (p *purger) hasFiles() bool {
	p.mu.Lock()
	defer p.mu.Unlock()

	has := false

	// Avoid calling Len() which iterates over the whole map.
	p.files.Range(func(k string, v TSMFile) bool {
		has = true
		return false // stop iteration after finding the first file
	})
	p.running = has
	return has
}

type tsmReaders []TSMFile

func (a tsmReaders) Len() int           { return len(a) }
func (a tsmReaders) Less(i, j int) bool { return a[i].Path() < a[j].Path() }
func (a tsmReaders) Swap(i, j int)      { a[i], a[j] = a[j], a[i] }
Repository: ionic-team/ionic-framework
File: core/src/components/range/range-interface.ts (TypeScript)

Code before:
export type KnobName = 'A' | 'B' | undefined;

export type RangeValue = number | {lower: number, upper: number};

export type PinFormatter = (value: number) => number | string;

export interface RangeChangeEventDetail {
  value: RangeValue;
}

export interface RangeChangeStartEventDetail {
  value: RangeValue;
}

export interface RangeChangeEndEventDetaill {
  value: RangeValue;
}

export interface RangeCustomEvent extends CustomEvent {
  detail: RangeChangeEventDetail | RangeChangeStartEventDetail | RangeChangeEndEventDetaill;
  target: HTMLIonRangeElement;
}


Review comment (suggestion):
~ Thoughts on re-using the existing `RangeChangeEventDetail`? Unsure of the value of separate interfaces, if the type signature is identical. 

Code after:
export type KnobName = 'A' | 'B' | undefined;

export type RangeValue = number | {lower: number, upper: number};

export type PinFormatter = (value: number) => number | string;

export interface RangeChangeEventDetail {
  value: RangeValue;
}

export interface RangeKnobMoveStartEventDetail {
  value: RangeValue;
}

export interface RangeKnobMoveEndEventDetail {
  value: RangeValue;
}

export interface RangeCustomEvent extends CustomEvent {
  detail: RangeChangeEventDetail | RangeKnobMoveStartEventDetail | RangeKnobMoveEndEventDetail;
  target: HTMLIonRangeElement;
}
Repository: ionic-team/ionic-framework
File: core/src/components/range/range.tsx (TypeScript)

Code before:
    if (this.didLoad) {
      this.setupGesture();
    }
  }

  disconnectedCallback() {
    if (this.gesture) {
      this.gesture.destroy();
      this.gesture = undefined;
    }
  }

  private handleKeyboard = (knob: KnobName, isIncrease: boolean) => {
    let step = this.step;
    step = step > 0 ? step : 1;
    step = step / (this.max - this.min);
    if (!isIncrease) {
      step *= -1;
    }
    if (knob === 'A') {
      this.ratioA = clamp(0, this.ratioA + step, 1);
    } else {
      this.ratioB = clamp(0, this.ratioB + step, 1);
    }

    this.ionChangeStart.emit({ value: this.value });
    this.updateValue();
    this.ionChangeEnd.emit({ value: this.value });
  }

  private getValue(): RangeValue {
    const value = this.value || 0;
    if (this.dualKnobs) {
      if (typeof value === 'object') {
        return value;
      }
      return {
        lower: 0,
        upper: value
      };
    } else {
      if (typeof value === 'object') {
        return value.upper;
      }
      return value;
    }
  }

  private emitStyle() {
    this.ionStyle.emit({
      'interactive': true,

Review comment (suggestion):
~ Consider using a local variable here, since we are accessing the same variable multiple times:
```tsx
const { value } = this;
this.ionChangeStart.emit({ value });
this.updateValue();
this.ionChangeEnd.emit({ value });
```

Code after:
     * then rangeSlider will be defined.
     */
    if (this.didLoad) {
      this.setupGesture();
    }
  }

  disconnectedCallback() {
    if (this.gesture) {
      this.gesture.destroy();
      this.gesture = undefined;
    }
  }

  private handleKeyboard = (knob: KnobName, isIncrease: boolean) => {
    const { ensureValueInBounds } = this;

    let step = this.step;
    step = step > 0 ? step : 1;
    step = step / (this.max - this.min);
    if (!isIncrease) {
      step *= -1;
    }
    if (knob === 'A') {
      this.ratioA = clamp(0, this.ratioA + step, 1);
    } else {
      this.ratioB = clamp(0, this.ratioB + step, 1);
    }

    this.ionKnobMoveStart.emit({ value: ensureValueInBounds(this.value) });
    this.updateValue();
    this.ionKnobMoveEnd.emit({ value: ensureValueInBounds(this.value) });
  }

  private getValue(): RangeValue {
    const value = this.value || 0;
    if (this.dualKnobs) {
      if (typeof value === 'object') {
        return value;
      }
      return {
        lower: 0,
        upper: value
      };
    } else {
      if (typeof value === 'object') {
        return value.upper;
      }
      return value;
    }
  }
Repository: ionic-team/ionic-framework
File: core/src/components/range/range-interface.ts (TypeScript)

Code before:
export type KnobName = 'A' | 'B' | undefined;

export type RangeValue = number | {lower: number, upper: number};

export type PinFormatter = (value: number) => number | string;

export interface RangeChangeEventDetail {
  value: RangeValue;
}

export interface RangeChangeStartEventDetail {
  value: RangeValue;
}

export interface RangeChangeEndEventDetaill {
  value: RangeValue;
}

export interface RangeCustomEvent extends CustomEvent {
  detail: RangeChangeEventDetail | RangeChangeStartEventDetail | RangeChangeEndEventDetaill;
  target: HTMLIonRangeElement;
}


Review comment (suggestion):
```suggestion
export interface RangeChangeEndEventDetail {
```

One extra "L"

Code after:
export type KnobName = 'A' | 'B' | undefined;

export type RangeValue = number | {lower: number, upper: number};

export type PinFormatter = (value: number) => number | string;

export interface RangeChangeEventDetail {
  value: RangeValue;
}

export interface RangeKnobMoveStartEventDetail {
  value: RangeValue;
}

export interface RangeKnobMoveEndEventDetail {
  value: RangeValue;
}

export interface RangeCustomEvent extends CustomEvent {
  detail: RangeChangeEventDetail | RangeKnobMoveStartEventDetail | RangeKnobMoveEndEventDetail;
  target: HTMLIonRangeElement;
}
Repository: ionic-team/ionic-framework
File: core/src/components/range/range.tsx (TypeScript)

Code before:
  /**
   * Emitted when the value property has changed.
   */
  @Event() ionChange!: EventEmitter<RangeChangeEventDetail>;

  /**
   * Emitted when the styles change.
   * @internal
   */
  @Event() ionStyle!: EventEmitter<StyleEventDetail>;

  /**
   * Emitted when the range has focus.
   */
  @Event() ionFocus!: EventEmitter<void>;

  /**
   * Emitted when the range loses focus.
   */
  @Event() ionBlur!: EventEmitter<void>;

  /**
   * Emitted when the user starts moving the range knob, whether through
   * mouse drag, touch gesture, or keyboard interaction.
   */
  @Event() ionChangeStart!: EventEmitter<RangeChangeStartEventDetail>;

  /**
   * Emitted when the user finishes moving the range knob, whether through
   * mouse drag, touch gesture, or keyboard interaction.
   */
  @Event() ionChangeEnd!: EventEmitter<RangeChangeEndEventDetaill>;

  private setupGesture = async () => {
    const rangeSlider = this.rangeSlider;
    if (rangeSlider) {
      this.gesture = (await import('../../utils/gesture')).createGesture({
        el: rangeSlider,
        gestureName: 'range',
        gesturePriority: 100,
        threshold: 0,
        onStart: ev => this.onStart(ev),
        onMove: ev => this.onMove(ev),
        onEnd: ev => this.onEnd(ev),
      });
      this.gesture.enable(!this.disabled);
    }
  }

  componentWillLoad() {
    /**

Review comment (suggestion):
Bikeshedding here, but thoughts on calling this `ionStart` and `ionEnd`? To me `ionChangeStart` implies that `ionChange` will fire after `ionChangeStart`. If you have a pointer on the knob, `ionChangeStart` will fire, but if you simply remove the point and do not move the knob, `ionChange` will not fire.

Code after:
  /**
   * Emitted when the value property has changed.
   */
  @Event() ionChange!: EventEmitter<RangeChangeEventDetail>;

  /**
   * Emitted when the styles change.
   * @internal
   */
  @Event() ionStyle!: EventEmitter<StyleEventDetail>;

  /**
   * Emitted when the range has focus.
   */
  @Event() ionFocus!: EventEmitter<void>;

  /**
   * Emitted when the range loses focus.
   */
  @Event() ionBlur!: EventEmitter<void>;

  /**
   * Emitted when the user starts moving the range knob, whether through
   * mouse drag, touch gesture, or keyboard interaction.
   */
  @Event() ionKnobMoveStart!: EventEmitter<RangeKnobMoveStartEventDetail>;

  /**
   * Emitted when the user finishes moving the range knob, whether through
   * mouse drag, touch gesture, or keyboard interaction.
   */
  @Event() ionKnobMoveEnd!: EventEmitter<RangeKnobMoveEndEventDetail>;

  private setupGesture = async () => {
    const rangeSlider = this.rangeSlider;
    if (rangeSlider) {
      this.gesture = (await import('../../utils/gesture')).createGesture({
        el: rangeSlider,
        gestureName: 'range',
        gesturePriority: 100,
        threshold: 0,
        onStart: ev => this.onStart(ev),
        onMove: ev => this.onMove(ev),
        onEnd: ev => this.onEnd(ev),
      });
      this.gesture.enable(!this.disabled);
    }
  }

  componentWillLoad() {
    /**
Repository: ionic-team/ionic-framework
File: core/src/components/range/range-interface.ts (TypeScript)

Code before:
export type KnobName = 'A' | 'B' | undefined;

export type RangeValue = number | {lower: number, upper: number};

export type PinFormatter = (value: number) => number | string;

export interface RangeChangeEventDetail {
  value: RangeValue;
}

export interface RangeChangeStartEventDetail {
  value: RangeValue;
}

export interface RangeChangeEndEventDetail {
  value: RangeValue;
}

export interface RangeCustomEvent extends CustomEvent {
  detail: RangeChangeEventDetail | RangeChangeStartEventDetail | RangeChangeEndEventDetail;
  target: HTMLIonRangeElement;
}


Review comment (suggestion):
These need to be documented in `readme.md`. You can add them above/under the `RangeChangeEventDetail` interface.

Code after:
export type KnobName = 'A' | 'B' | undefined;

export type RangeValue = number | {lower: number, upper: number};

export type PinFormatter = (value: number) => number | string;

export interface RangeChangeEventDetail {
  value: RangeValue;
}

export interface RangeKnobMoveStartEventDetail {
  value: RangeValue;
}

export interface RangeKnobMoveEndEventDetail {
  value: RangeValue;
}

export interface RangeCustomEvent extends CustomEvent {
  detail: RangeChangeEventDetail | RangeKnobMoveStartEventDetail | RangeKnobMoveEndEventDetail;
  target: HTMLIonRangeElement;
}