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

Add update dyanmo functionality #4

Merged
merged 1 commit into from
Aug 24, 2017

Conversation

Calum-Campbell
Copy link

@Calum-Campbell Calum-Campbell commented Aug 22, 2017

  • Adds the functionality for the dynamo dependancy to update a switch remotely
  • Also adds functionality to "get" the state of the switches live and surpass the scheduler
  • Bumps version to 0.4 (still needs to be published)

Used in :

case class Switch(name: String, default: Boolean = false) {
@volatile private var state: Boolean = default

def enabled: Boolean = state

def enabled_=(value: Boolean) = state = value

def toStringAttribute() = {
new AttributeValue().withS(this.name)
Copy link

Choose a reason for hiding this comment

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

Can you not store boolean in DynamoDB?

Copy link
Author

@Calum-Campbell Calum-Campbell Aug 23, 2017

Choose a reason for hiding this comment

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

This is just how the library / switchboard was originally written! Maybe you didn't used to be able to store booleans.. Not sure.

def updateDynamo(switches : List[Switch]) = {
val listWR = switches.map(switch => {
new WriteRequest(new PutRequest(Map("name" -> switch.toStringAttribute(), "enabled" -> switch.toNumAttributeValue()).asJava))
}).asJava
Copy link

Choose a reason for hiding this comment

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

No need for the inside {}

val results = dynamoDbClient.scan(new ScanRequest(dynamoDbTableName)).getItems.asScala.toList.map(_.asScala.toMap)
results.map(item => {
Switch(item("name").getS, item("enabled").getN == "1")
})
Copy link

Choose a reason for hiding this comment

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

No need for the inside {}
You also don't need the intermediary val results and the double map. What about:

dynamoDbClient
  .scan(new ScanRequest(dynamoDbTableName))
  .getItems.asScala.toList
  .map { 
    val item = _.asScala.toMap
    Switch(item("name").getS, item("enabled").getN == "1")
  }

new AttributeValue().withS(this.name)
}

def toNumAttributeValue() = {
Copy link
Contributor

@nicl nicl Aug 23, 2017

Choose a reason for hiding this comment

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

This seems like an odd method. It converts the default value to a string containing 1 or 0.

Perhaps a better name might be defaultAsAttributeValue?

But even better might be to inline this logic, or if used more than once define a more generic and pure method in a companion or helper object. E.g.

def asAttributeValue(b: Boolean) = {
  val str = if(b) "1" else "0"
  new AttributeValue().withN(str)
}

A minor tweak as well, but if a method has no side effects it is idiomatic in Scala to declare it without the () to indicate this. E.g. toNumAttributeValue = {...}

dynamoDbClient.batchWriteItem(Map(dynamoDbTableName -> listWR).asJava)
} catch {
case e: Exception => println(e)
case _ => println("Something went wrong")
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we'd log this, but I can see adding support for a logger might be easier in a separate PR.

Copy link
Author

Choose a reason for hiding this comment

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

Yep changed to error and warn for now?

Copy link
Contributor

@nicl nicl left a comment

Choose a reason for hiding this comment

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

Added a few comments, but the only significant issue is logging which it would be good to add, but we can think about this separately.

@Calum-Campbell Calum-Campbell force-pushed the cc-add-update-functionality branch 2 times, most recently from ebad0ae to 96c7ae7 Compare August 24, 2017 14:20
@Calum-Campbell Calum-Campbell merged commit 08f3f8a into master Aug 24, 2017
@Calum-Campbell Calum-Campbell deleted the cc-add-update-functionality branch August 24, 2017 14:48
@Calum-Campbell Calum-Campbell restored the cc-add-update-functionality branch August 24, 2017 14:48
@mariogalic mariogalic deleted the cc-add-update-functionality branch September 26, 2019 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants