Skip to content

Commit

Permalink
fix shard lookup for Integer.MIN_VALUE (#1452)
Browse files Browse the repository at this point in the history
Before it was relying on `abs` to get a non-negative value
for computing an index to the array. This breaks when the
int value of the id is `Integer.MIN_VALUE`. Update it to
explicitly clear the sign bit to ensure the value will
always be non-negative.
  • Loading branch information
brharrington authored Aug 2, 2022
1 parent 483dfc5 commit 1239586
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 5 deletions.
16 changes: 12 additions & 4 deletions atlas-core/src/main/scala/com/netflix/atlas/core/util/Shards.scala
Original file line number Diff line number Diff line change
Expand Up @@ -147,13 +147,13 @@ object Shards {

/** Return the instance that should receive the data associated with `id`. */
def instanceForId(id: BigInteger): T = {
val i = math.abs(id.intValue())
val i = nonNegative(id.intValue())
instanceForIndex(i)
}

/** Return the instance that should receive the data associated with `id`. */
def instanceForId(id: ItemId): T = {
val i = math.abs(id.intValue)
val i = nonNegative(id.intValue)
instanceForIndex(i)
}

Expand All @@ -176,7 +176,7 @@ object Shards {

/** Return true if this instance should include data for `id`. */
def containsId(id: BigInteger): Boolean = {
val i = math.abs(id.intValue())
val i = nonNegative(id.intValue())
containsIndex(i)
}

Expand All @@ -196,7 +196,7 @@ object Shards {

/** Return the instances that should receive the data associated with `id`. */
def instancesForId(id: BigInteger): List[T] = {
val i = math.abs(id.intValue())
val i = nonNegative(id.intValue())
instancesForIndex(i)
}

Expand All @@ -209,4 +209,12 @@ object Shards {
}
}
}

/**
* Returns the absolute value for the number unless it is Integer.MIN_VALUE, in which case
* it will return 0.
*/
private[util] def nonNegative(v: Int): Int = {
math.abs(v) & 0x7fffffff
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,13 @@
*/
package com.netflix.atlas.core.util

import java.util.UUID
import com.netflix.atlas.core.model.ItemId

import java.util.UUID
import munit.FunSuite

import java.util.Random

class ShardsSuite extends FunSuite {

private def createGroups(n: Int, sz: Int): List[Shards.Group[Int]] = {
Expand Down Expand Up @@ -95,6 +98,24 @@ class ShardsSuite extends FunSuite {
}
}

test("intValue of id is Integer.MIN_VALUE") {
// Verify it doesn't fail with:
// java.lang.IllegalArgumentException: requirement failed: index cannot be negative
val id = ItemId("016adce025b0485b9f581d071961de1480000000")
val groups = createGroups(10, 10)
val mapper = Shards.mapper(groups)
mapper.instanceForId(id)
}

test("intValue of bigint is Integer.MIN_VALUE") {
// Verify it doesn't fail with:
// java.lang.IllegalArgumentException: requirement failed: index cannot be negative
val id = ItemId("016adce025b0485b9f581d071961de1480000000").toBigInteger
val groups = createGroups(10, 10)
val mapper = Shards.mapper(groups)
mapper.instanceForId(id)
}

test("uneven groups") {
val groups = List(
Shards.Group("a", Array(0, 1)),
Expand Down Expand Up @@ -242,4 +263,23 @@ class ShardsSuite extends FunSuite {
counts.foreach((_, v) => sum += v)
assert(sum >= 20000 + 20000 / 3)
}

test("nonNegative max") {
assertEquals(Shards.nonNegative(Integer.MAX_VALUE), Integer.MAX_VALUE)
}

test("nonNegative min") {
assertEquals(Shards.nonNegative(Integer.MIN_VALUE), 0)
}

test("nonNegative random") {
val r = new Random()
(0 until 10_000).foreach { i =>
val v = r.nextInt()
assert(Shards.nonNegative(v) >= 0)
if (v != Integer.MIN_VALUE) {
assertEquals(Shards.nonNegative(v), math.abs(v))
}
}
}
}

0 comments on commit 1239586

Please sign in to comment.