Skip to content

Commit

Permalink
Fix lint issues in target allocator (#1090)
Browse files Browse the repository at this point in the history
* fix lint issues in target allocator files

squash of all current commits on this branch

* remove addtl golangci config

* remove version.txt
  • Loading branch information
Kristina Pathak authored Oct 3, 2022
1 parent f7aafc5 commit 77673ab
Show file tree
Hide file tree
Showing 24 changed files with 348 additions and 75 deletions.
4 changes: 4 additions & 0 deletions .github/workflows/continuous-integration.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ jobs:
lint:
name: Code standards (linting)
runs-on: ubuntu-20.04
strategy:
matrix:
workdir: [".", "./cmd/otel-allocator"]
steps:
- name: Set up Go
uses: actions/setup-go@v3
Expand All @@ -44,6 +47,7 @@ jobs:
with:
args: -v
version: v1.48
working-directory: ${{ matrix.workdir }}

security:
name: Security
Expand Down
2 changes: 0 additions & 2 deletions .golangci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ linters-settings:
locale: US
govet:
disable-all: true
enable:
- fieldalignment

linters:
enable:
Expand Down
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ vet:
.PHONY: lint
lint:
golangci-lint run
cd cmd/otel-allocator && golangci-lint run

# Generate code
.PHONY: generate
Expand Down
26 changes: 19 additions & 7 deletions cmd/otel-allocator/allocation/consistent_hashing.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package allocation

import (
Expand Down Expand Up @@ -56,8 +70,8 @@ func newConsistentHashingAllocator(log logr.Logger) Allocator {

// addTargetToTargetItems assigns a target to the collector based on its hash and adds it to the allocator's targetItems
// This method is called from within SetTargets and SetCollectors, which acquire the needed lock.
// This is only called after the collectors are cleared or when a new target has been found in the tempTargetMap
// INVARIANT: c.collectors must have at least 1 collector set
// This is only called after the collectors are cleared or when a new target has been found in the tempTargetMap.
// INVARIANT: c.collectors must have at least 1 collector set.
func (c *consistentHashingAllocator) addTargetToTargetItems(target *TargetItem) {
// Check if this is a reassignment, if so, decrement the previous collector's NumTargets
if previousColName, ok := c.collectors[target.CollectorName]; ok {
Expand All @@ -78,8 +92,8 @@ func (c *consistentHashingAllocator) addTargetToTargetItems(target *TargetItem)
}

// handleTargets receives the new and removed targets and reconciles the current state.
// Any removals are removed from the allocator's targetItems and unassigned from the corresponding collector
// Any net-new additions are assigned to the next available collector
// Any removals are removed from the allocator's targetItems and unassigned from the corresponding collector.
// Any net-new additions are assigned to the next available collector.
func (c *consistentHashingAllocator) handleTargets(diff diff.Changes[*TargetItem]) {
// Check for removals
for k, target := range c.targetItems {
Expand All @@ -105,7 +119,7 @@ func (c *consistentHashingAllocator) handleTargets(diff diff.Changes[*TargetItem
}

// handleCollectors receives the new and removed collectors and reconciles the current state.
// Any removals are removed from the allocator's collectors. New collectors are added to the allocator's collector map
// Any removals are removed from the allocator's collectors. New collectors are added to the allocator's collector map.
// Finally, update all targets' collectors to match the consistent hashing.
func (c *consistentHashingAllocator) handleCollectors(diff diff.Changes[*Collector]) {
// Clear removed collectors
Expand Down Expand Up @@ -146,7 +160,6 @@ func (c *consistentHashingAllocator) SetTargets(targets map[string]*TargetItem)
if len(targetsDiff.Additions()) != 0 || len(targetsDiff.Removals()) != 0 {
c.handleTargets(targetsDiff)
}
return
}

// SetCollectors sets the set of collectors with key=collectorName, value=Collector object.
Expand All @@ -170,7 +183,6 @@ func (c *consistentHashingAllocator) SetCollectors(collectors map[string]*Collec
if len(collectorsDiff.Additions()) != 0 || len(collectorsDiff.Removals()) != 0 {
c.handleCollectors(collectorsDiff)
}
return
}

// TargetItems returns a shallow copy of the targetItems map.
Expand Down
26 changes: 20 additions & 6 deletions cmd/otel-allocator/allocation/consistent_hashing_test.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package allocation

import (
Expand All @@ -7,7 +21,7 @@ import (
)

func TestCanSetSingleTarget(t *testing.T) {
cols := makeNCollectors(3, 0, 0)
cols := makeNCollectors(3, 0)
c := newConsistentHashingAllocator(logger)
c.SetCollectors(cols)
c.SetTargets(makeNNewTargets(1, 3, 0))
Expand All @@ -21,7 +35,7 @@ func TestCanSetSingleTarget(t *testing.T) {
func TestRelativelyEvenDistribution(t *testing.T) {
numCols := 15
numItems := 10000
cols := makeNCollectors(numCols, 0, 0)
cols := makeNCollectors(numCols, 0)
var expectedPerCollector = float64(numItems / numCols)
expectedDelta := (expectedPerCollector * 1.5) - expectedPerCollector
c := newConsistentHashingAllocator(logger)
Expand All @@ -38,15 +52,15 @@ func TestRelativelyEvenDistribution(t *testing.T) {
}

func TestFullReallocation(t *testing.T) {
cols := makeNCollectors(10, 0, 0)
cols := makeNCollectors(10, 0)
c := newConsistentHashingAllocator(logger)
c.SetCollectors(cols)
c.SetTargets(makeNNewTargets(10000, 10, 0))
actualTargetItems := c.TargetItems()
assert.Len(t, actualTargetItems, 10000)
actualCollectors := c.Collectors()
assert.Len(t, actualCollectors, 10)
newCols := makeNCollectors(10, 0, 10)
newCols := makeNCollectors(10, 10)
c.SetCollectors(newCols)
updatedTargetItems := c.TargetItems()
assert.Len(t, updatedTargetItems, 10000)
Expand All @@ -63,15 +77,15 @@ func TestNumRemapped(t *testing.T) {
numInitialCols := 15
numFinalCols := 16
expectedDelta := float64((numFinalCols - numInitialCols) * (numItems / numFinalCols))
cols := makeNCollectors(numInitialCols, 0, 0)
cols := makeNCollectors(numInitialCols, 0)
c := newConsistentHashingAllocator(logger)
c.SetCollectors(cols)
c.SetTargets(makeNNewTargets(numItems, numInitialCols, 0))
actualTargetItems := c.TargetItems()
assert.Len(t, actualTargetItems, numItems)
actualCollectors := c.Collectors()
assert.Len(t, actualCollectors, numInitialCols)
newCols := makeNCollectors(numFinalCols, 0, 0)
newCols := makeNCollectors(numFinalCols, 0)
c.SetCollectors(newCols)
updatedTargetItems := c.TargetItems()
assert.Len(t, updatedTargetItems, numItems)
Expand Down
14 changes: 14 additions & 0 deletions cmd/otel-allocator/allocation/http.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package allocation

import (
Expand Down
14 changes: 14 additions & 0 deletions cmd/otel-allocator/allocation/http_test.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package allocation

import (
Expand Down
30 changes: 21 additions & 9 deletions cmd/otel-allocator/allocation/least_weighted.go
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package allocation

import (
Expand Down Expand Up @@ -61,8 +75,8 @@ func (allocator *leastWeightedAllocator) Collectors() map[string]*Collector {

// findNextCollector finds the next collector with fewer number of targets.
// This method is called from within SetTargets and SetCollectors, whose caller
// acquires the needed lock. This method assumes there are is at least 1 collector set
// INVARIANT: allocator.collectors must have at least 1 collector set
// acquires the needed lock. This method assumes there are is at least 1 collector set.
// INVARIANT: allocator.collectors must have at least 1 collector set.
func (allocator *leastWeightedAllocator) findNextCollector() *Collector {
var col *Collector
for _, v := range allocator.collectors {
Expand All @@ -78,8 +92,8 @@ func (allocator *leastWeightedAllocator) findNextCollector() *Collector {

// addTargetToTargetItems assigns a target to the next available collector and adds it to the allocator's targetItems
// This method is called from within SetTargets and SetCollectors, which acquire the needed lock.
// This is only called after the collectors are cleared or when a new target has been found in the tempTargetMap
// INVARIANT: allocator.collectors must have at least 1 collector set
// This is only called after the collectors are cleared or when a new target has been found in the tempTargetMap.
// INVARIANT: allocator.collectors must have at least 1 collector set.
func (allocator *leastWeightedAllocator) addTargetToTargetItems(target *TargetItem) {
chosenCollector := allocator.findNextCollector()
targetItem := &TargetItem{
Expand All @@ -95,8 +109,8 @@ func (allocator *leastWeightedAllocator) addTargetToTargetItems(target *TargetIt
}

// handleTargets receives the new and removed targets and reconciles the current state.
// Any removals are removed from the allocator's targetItems and unassigned from the corresponding collector
// Any net-new additions are assigned to the next available collector
// Any removals are removed from the allocator's targetItems and unassigned from the corresponding collector.
// Any net-new additions are assigned to the next available collector.
func (allocator *leastWeightedAllocator) handleTargets(diff diff.Changes[*TargetItem]) {
// Check for removals
for k, target := range allocator.targetItems {
Expand All @@ -122,7 +136,7 @@ func (allocator *leastWeightedAllocator) handleTargets(diff diff.Changes[*Target
}

// handleCollectors receives the new and removed collectors and reconciles the current state.
// Any removals are removed from the allocator's collectors. New collectors are added to the allocator's collector map
// Any removals are removed from the allocator's collectors. New collectors are added to the allocator's collector map.
// Finally, any targets of removed collectors are reallocated to the next available collector.
func (allocator *leastWeightedAllocator) handleCollectors(diff diff.Changes[*Collector]) {
// Clear removed collectors
Expand Down Expand Up @@ -163,7 +177,6 @@ func (allocator *leastWeightedAllocator) SetTargets(targets map[string]*TargetIt
if len(targetsDiff.Additions()) != 0 || len(targetsDiff.Removals()) != 0 {
allocator.handleTargets(targetsDiff)
}
return
}

// SetCollectors sets the set of collectors with key=collectorName, value=Collector object.
Expand All @@ -187,7 +200,6 @@ func (allocator *leastWeightedAllocator) SetCollectors(collectors map[string]*Co
if len(collectorsDiff.Additions()) != 0 || len(collectorsDiff.Removals()) != 0 {
allocator.handleCollectors(collectorsDiff)
}
return
}

func newLeastWeightedAllocator(log logr.Logger) Allocator {
Expand Down
Loading

0 comments on commit 77673ab

Please sign in to comment.