Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix lint issues in target allocator #1090

Merged
merged 3 commits into from
Oct 3, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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