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

AWS SDK for Java: version 1.x --> version 2.1.0 #8

Open
wants to merge 23 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@sullis
Collaborator

sullis commented Dec 7, 2017

No description provided.

@sullis

This comment has been minimized.

Collaborator

sullis commented Dec 7, 2017

Work In Progress :)

@kiiadi

Looking good so far - few comments for some 'features' of the SDK you might want to look at.

@@ -174,7 +177,7 @@ object CloudWatchLogsClientImpl {
val Logger = new OpenLoggable {}
private
val awsClient = new AWSLogsClient()
val awsClient = CloudWatchLogsAsyncClient.builder.build

This comment has been minimized.

@kiiadi

kiiadi Dec 8, 2017

You should just able to do CloudWatchLogsAsyncClient.create

This comment has been minimized.

@sullis

sullis Dec 14, 2017

Collaborator

Awesome. Thanks.

This comment has been minimized.

@sullis

sullis Dec 14, 2017

Collaborator

Done

awsClient.putLogEvents(request.withSequenceToken(nstp.getNextSequenceToken(request.getLogGroupName, request.getLogStreamName)))
def putLogEvents(request: PutLogEventsRequest)(implicit nstp: NextSequenceTokenPersistor, ec: ExecutionContext): Future[PutLogEventsResponse] = {
val response: Future[PutLogEventsResponse] = (try {
awsClient.putLogEvents(request.toBuilder.sequenceToken(nstp.getNextSequenceToken(request.logGroupName, request.logStreamName)).build).toScala

This comment has been minimized.

@kiiadi

kiiadi Dec 8, 2017

here you can use copy instead of doing toBuilder and then build - but then I'm not sure how well Scala inter-ops with Java FunctionalInterfaces can you define them as Lambdas?

With copy this line:

request.toBuilder.sequenceToken(nstp.getNextSequenceToken(request.logGroupName, request.logStreamName)).build

can become

request.copy(r -> r.sequenceToken(nstp.getNextSequenceToken(request.logGroupName, request.logStreamName))

This comment has been minimized.

@sullis

sullis Dec 14, 2017

Collaborator

I am going to keep 'toBuilder' here because I want to stay compatible with Scala 2.11 and Scala 2.12

} catch {
case e: ResourceNotFoundException if e.getMessage.toLowerCase.contains("log group") =>
awsClient.createLogGroup(
new CreateLogGroupRequest()
.withLogGroupName(request.getLogGroupName)
CreateLogGroupRequest.builder()

This comment has been minimized.

@kiiadi

kiiadi Dec 8, 2017

Again with preview-6 you can use our "Consumer" method overloads to make this:

awsClient.createLogGroup(r -> r.logGroupName(request.logGroupName))

Instead of having to worry about locating the builder and calling build.

This comment has been minimized.

@sullis

sullis Dec 14, 2017

Collaborator

I'm not sure I want to use java.util.function.Consumer in Scala code. I'm going to keep the 'builder' / 'build' code for now.

withUnit(metricUnit).
withStatisticValues(statsValues).
withTimestamp(new Date())
def mdBuilder = MetricDatum.builder. // def, not val, we mutate it

This comment has been minimized.

@kiiadi

kiiadi Dec 8, 2017

Interesting that you're holding a copy of the builder - that's obviously not going to be thread-safe so if you called the mutation code below in multiple threads you might get interesting results.

This comment has been minimized.

@andreyk0

andreyk0 Dec 8, 2017

Contributor

@kiiadi can you explain? This is a def, not a val, comment highlights the mutation use case.

This comment has been minimized.

@kiiadi

kiiadi Dec 8, 2017

@andreyk0 it's been a while since I've used Scala - by defining it as a def does this get invoked for each call? ie: a new builder gets created every time - if that's the case the mutation doesn't matter I suppose.

The other way you could do this would be to assign an immutable version to a val and then use the copy operation to apply the dimensions below.

val datatum = MetricDatum.builder.metricName(metricName).unit(metricUnit).(...).build

Then when you go to use it below

metricDimensions.map(ds => metricDatum.copy(dt => dt. dimensions(ds.asJava)))

This comment has been minimized.

@andreyk0

andreyk0 Dec 8, 2017

Contributor

Yep, def is a "function" and val is a "value", so, def basically gets called every time.
The reason I prefer def to .copy is because .copy needs to be consistently applied at all the call sites. def (at the expense of extra object creation) basically guarantees that there will be no concurrent modifications.

This comment has been minimized.

@kiiadi
build.sbt Outdated
val awsLibVersion = "1.11.243"

This comment has been minimized.

@andreyk0

andreyk0 Dec 8, 2017

Contributor

How should we approach this kind of version jump? Just refer people to older version if they are on an older AWS SDK or should we consider branching for a while, to give projects time to upgrade?

This comment has been minimized.

@sullis

sullis Dec 14, 2017

Collaborator

I think we want to have a branch for AWS SDK v2 and a different branch for AWS SDK v1.
Our SDK V2 implementation will be a major version bump.

import com.amazonaws.services.cloudwatch.model.{MetricDatum, PutMetricDataRequest}
import software.amazon.awssdk.services.cloudwatch.CloudWatchAsyncClient
import software.amazon.awssdk.services.cloudwatch.model.{MetricDatum, PutMetricDataRequest}
// import com.amazonaws.services.cloudwatch.{AmazonCloudWatchClient, AmazonCloudWatchClientBuilder}

This comment has been minimized.

@andreyk0

andreyk0 Dec 8, 2017

Contributor

Do we need commented-out lines?

This comment has been minimized.

@sullis

sullis Dec 14, 2017

Collaborator

Done.

@andreyk0

This comment has been minimized.

Contributor

andreyk0 commented Dec 8, 2017

👍 Looks great overall, few small comments inline.

@kiiadi

kiiadi approved these changes Dec 11, 2017

withUnit(metricUnit).
withStatisticValues(statsValues).
withTimestamp(new Date())
def mdBuilder = MetricDatum.builder. // def, not val, we mutate it

This comment has been minimized.

@kiiadi

@sullis sullis changed the title from AWS SDK for Java: version 1.x --> version 2.x to AWS SDK for Java: version 1.x --> version 2.1.0 Nov 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment