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.

Sign up for GitHub

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 getNextAvailableAddress to use local maximums instead of just last a... #168

Merged
merged 6 commits into from
Jun 23, 2014
Merged

fix getNextAvailableAddress to use local maximums instead of just last a... #168

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1ec999d
fix getNextAvailableAddress to use local maximums instead of just las…
byxorna Jun 20, 2014
8a5408e
use sorted set instead
byxorna Jun 21, 2014
eee92c3
use headOption and if instead
byxorna Jun 21, 2014
5b878c0
use more elegant flatMap and for comprehension
byxorna Jun 21, 2014
3be2e75
use lazy streams
byxorna Jun 23, 2014
f706731
make the function actually work now
byxorna Jun 23, 2014
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
2 changes: 1 addition & 1 deletion app/models/IpAddresses.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ object IpAddresses extends IpAddressStorage[IpAddresses] with IpAddressCacheMana
case failed =>
logger.info("Address cache dirty. Failed cache lookup so using DB")
populateCacheIfNeeded(scope, true)
getCurrentMaxAddress(calc.minAddressAsLong, calc.maxAddressAsLong)
getCurrentLowestLocalMaxAddress(calc.minAddressAsLong, calc.maxAddressAsLong)
}
val address: Long = calc.nextAvailableAsLong(currentMax)
(gateway, address, netmask)
Expand Down
45 changes: 35 additions & 10 deletions app/models/shared/IpAddressable.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ trait IpAddressable extends ValidatedEntity[Long] {
def getId(): Long = id
def getAssetId(): Long = asset_id
def getAsset(): Asset = Asset.findById(getAssetId()).get

}

trait IpAddressStorage[T <: IpAddressable] extends Schema with AnormAdapter[T] {
Expand Down Expand Up @@ -75,15 +75,15 @@ trait IpAddressStorage[T <: IpAddressable] extends Schema with AnormAdapter[T] {
}

def getNextAvailableAddress(overrideStart: Option[String] = None)(implicit scope: Option[String]): Tuple3[Long,Long,Long] = {
//this is used by ip allocation without pools (i.e. IPMI)
val network = getNetwork
val startAt = overrideStart.orElse(getStartAddress)
val calc = IpAddressCalc(network, startAt)
val gateway: Long = getGateway().getOrElse(calc.minAddressAsLong)
val netmask: Long = calc.netmaskAsLong
val currentMax: Option[Long] = getCurrentMaxAddress(
calc.minAddressAsLong, calc.maxAddressAsLong
)
val address: Long = calc.nextAvailableAsLong(currentMax)
// look for the local maximum address (i.e. the last used address in a continuous sequence from startAddress)
val localMax: Option[Long] = getCurrentLowestLocalMaxAddress(calc.minAddressAsLong, calc.maxAddressAsLong)
val address: Long = calc.nextAvailableAsLong(localMax)
(gateway, address, netmask)
}

Expand All @@ -95,7 +95,7 @@ trait IpAddressStorage[T <: IpAddressable] extends Schema with AnormAdapter[T] {
var i = 0
do {
try {
res = Some(f(i))
res = Some(f(i))
} catch {
case e: SQLException =>
logger.info("createAddressWithRetry attempt %d: %s".format((i + 1), e.getMessage))
Expand All @@ -115,14 +115,39 @@ trait IpAddressStorage[T <: IpAddressable] extends Schema with AnormAdapter[T] {
)
}

protected def getCurrentMaxAddress(minAddress: Long, maxAddress: Long)(implicit scope: Option[String]): Option[Long] = inTransaction {
from(tableDef)(t =>
/*
* returns the lowest last used address in a continuous sequence from minAddress
* If maxAddress is the same as the lowest last used address, we return None which signifies
* that you should start allocating addresses from the start of the range.
*
* Ex: For a range 0L..20L, used addresses List(1,2,3,4,5,13,14,15,19,20), the result will be Some(5)
* For a range 0L..20L, used addresses List(5,6,7,8,19,20), the result will be Some(8)
* For a range 0L..20L, used addresses List(17,18,19,20), the result will be None (allocate from beginning)
*/
protected def getCurrentLowestLocalMaxAddress(minAddress: Long, maxAddress: Long)(implicit scope: Option[String]): Option[Long] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably the way to write this entire function

scala> val x:Seq[Long]=List(1,2,3,8,9)
x: Seq[Long] = List(1, 2, 3, 8, 9)

scala> val first = for { i <- 0 to x.size -2
     |   curr = x(i)
     |   next = x(i+1)
     |   if (next > curr + 1)
     | } yield  curr
first: scala.collection.immutable.IndexedSeq[Long] = Vector(3)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I use a SortedSet, this doesnt work, as apply() on a SortedSet is a boolean (contains). I suppose though there isnt a need for a sorted set though, so Ill use your solution with normal lists that are sorted by the DB.

O(n) is better than O(nlog(n)) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this comprehension doesnt cover the case where sortedAddresses.size<=2 (meaning the range is 0..0) when using Stream.range, because it is not inclusive. Interestingly:

scala> Range(0,0)
res4: scala.collection.immutable.Range = Range()
scala> 0 to 0
res8: scala.collection.immutable.Range.Inclusive = Range(0)
scala> Stream.range(0,0)
res23: scala.collection.immutable.Stream[Int] = Stream()

I havent been able to find out how to make a Stream range be inclusive. Any ideas? This current iteration of code eats it after allocating the first address, because it thinks there are no localMaxima still :(

val sortedAddresses = from(tableDef)(t =>
where(
(t.address gte minAddress) and
(t.address lte maxAddress)
)
compute(max(t.address))
)
select(t.address)
orderBy(t.address asc)
).toList
val localMaximaAddresses = for (
localMax <- sortedAddresses if !sortedAddresses.contains(localMax + 1)
) yield localMax

localMaximaAddresses.isEmpty match {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

localMaximaAddresses.headOption.flatMap( localMax => ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah awesome. I knew there had to be something like that, but totally forgot about this method. Thanks, that will clean this up

case true => None
case false => {
val localMax = localMaximaAddresses.head
localMax match {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is possibly better off as an if, you aren't using limit anywhere. Code generated for pattern matching especially with guards is worse than an if stmt

tumblr-MacBookPro-78290b:~ bhaskar$ cat t.scala t1.scala
object App {
  def x(a:Int) = {
    a match {
      case v if v < 20 => None
      case _ => Some(20)
    }
  }
}
object App {
  def x(a:Int) = {
    if (a < 20)
      None
    else
     Some(20)
  }
}

tumblr-MacBookPro-78290b:~ bhaskar$ /Users/bhaskar/apps/scala-2.10.3/bin/scalac -Xprint:jvm t.scala
[[syntax trees at end of                       jvm]] // t.scala
package <empty> {
  object App extends Object {
    def x(a: Int): Option = {
      case <synthetic> val x1: Int = a;
      (x1: Int) match {
        case _ => if (x1.<(20))
          scala.None
        else
          new Some(scala.Int.box(20))
      }
    };
    def <init>(): App.type = {
      App.super.<init>();
      ()
    }
  }
}

tumblr-MacBookPro-78290b:~ bhaskar$ /Users/bhaskar/apps/scala-2.10.3/bin/scalac -Xprint:jvm t1.scala
[[syntax trees at end of                       jvm]] // t1.scala
package <empty> {
  object App extends Object {
    def x(a: Int): Option = if (a.<(20))
      scala.None
    else
      new Some(scala.Int.box(20));
    def <init>(): App.type = {
      App.super.<init>();
      ()
    }
  }
}

//if we are at the end of our range, start from the beginning
case limit if localMax == maxAddress => None
case _ => Some(localMax)
}
}
}
}

protected def getGateway()(implicit scope: Option[String]): Option[Long] = getConfig() match {
Expand Down