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

Use the SymbolProvider and Model directly from the Smithy plugins #1438

Conversation

david-perez
Copy link
Contributor

Despite all the decorators and protocols that we have, there aren't any
RustCodegenDecorators or ProtocolGeneratorFactorys that are
modifying the base symbol provider that the Smithy plugin provides. So
we can use directly the base symbol provider everywhere and remove the
methods from these interfaces to make the code a little bit simpler.

Likewise, there isn't a single ProtocolGeneratorFactory that
transforms the base model that the Smithy plugin provides.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@david-perez david-perez requested review from a team as code owners June 3, 2022 12:43
@david-perez david-perez mentioned this pull request Jun 3, 2022
@github-actions
Copy link

github-actions bot commented Jun 3, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec -1.39% 85315.53 86518.71
Total requests -1.41% 7683967 7793827
Total errors NaN% 0 0
Total successes -1.41% 7683967 7793827
Average latency ms 15.60% 1.26 1.09
Minimum latency ms 0.00% 0.02 0.02
Maximum latency ms -13.85% 23.94 27.79
Stdev latency ms 8.82% 2.22 2.04
Transfer Mb -1.41% 798.75 810.17
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

@@ -36,8 +35,6 @@ class ServerAwsJsonFactory(private val version: AwsJsonVersion) : ProtocolGenera
override fun buildProtocolGenerator(codegenContext: CodegenContext): ServerHttpBoundProtocolGenerator =
ServerHttpBoundProtocolGenerator(codegenContext, protocol(codegenContext))

override fun transformModel(model: Model): Model = model
Copy link
Collaborator

Choose a reason for hiding this comment

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

I used this yesterday to augment the S3 model with some extra metadata—I think we should keep these, especially since the default implementation means that there isn't any cost to having it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You used the transformModel from the decorators, not this one. I accidentally removed the invocation of the former, but I didn't mean to, that's why CI failed. I've now fixed that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah! got it

Despite all the decorators and protocols that we have, there aren't any
`RustCodegenDecorator`s or `ProtocolGeneratorFactory`s that are
modifying the base symbol provider that the Smithy plugin provides. So
we can use directly the base symbol provider everywhere and remove the
methods from these interfaces to make the code a little bit simpler.

Likewise, there isn't a single `ProtocolGeneratorFactory` that
transforms the base model that the Smithy plugin provides.
@david-perez david-perez force-pushed the davidpz/use-the-_symbolprovider_-and-_model_-directly-from-the-smithy-plugins branch from 689be8f to 862f073 Compare June 3, 2022 17:01
@github-actions
Copy link

github-actions bot commented Jun 3, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec -1.28% 78034.38 79045.22
Total requests -1.30% 7027759 7120354
Total errors NaN% 0 0
Total successes -1.30% 7027759 7120354
Average latency ms 2.13% 0.96 0.94
Minimum latency ms 0.00% 0.02 0.02
Maximum latency ms 0.34% 23.78 23.7
Stdev latency ms -1.83% 1.61 1.64
Transfer Mb -1.30% 730.54 740.16
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

import software.amazon.smithy.rust.codegen.smithy.generators.LibRsCustomization
import software.amazon.smithy.rust.codegen.smithy.generators.ManifestCustomizations
import software.amazon.smithy.rust.codegen.smithy.generators.client.FluentClientDecorator
import software.amazon.smithy.rust.codegen.smithy.generators.config.ConfigCustomization
import software.amazon.smithy.rust.codegen.smithy.protocols.ProtocolMap
import software.amazon.smithy.rust.codegen.util.deepMergeWith
import java.util.ServiceLoader
import java.util.*
Copy link
Contributor

Choose a reason for hiding this comment

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

Why hasn't the linter complained about *?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What linter?

Copy link
Collaborator

Choose a reason for hiding this comment

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

kotlin style check

@github-actions
Copy link

github-actions bot commented Jun 9, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement Deviation Current Old
Requests/sec -1.85% 88658.41 90328.2
Total requests -1.81% 7987897 8134867
Total errors NaN% 0 0
Total successes -1.81% 7987897 8134867
Average latency ms -3.33% 1.16 1.2
Minimum latency ms 0.00% 0.02 0.02
Maximum latency ms -0.21% 23.99 24.04
Stdev latency ms -1.83% 2.14 2.18
Transfer Mb -1.81% 830.35 845.62
Connect errors NaN% 0 0
Read errors NaN% 0 0
Write errors NaN% 0 0
Status errors (not 2xx/3xx) NaN% 0 0
Timeout errors NaN% 0 0

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the cleanup!

@github-actions
Copy link

github-actions bot commented Jun 9, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

…mbolprovider_-and-_model_-directly-from-the-smithy-plugins
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

…mbolprovider_-and-_model_-directly-from-the-smithy-plugins
@david-perez david-perez force-pushed the davidpz/use-the-_symbolprovider_-and-_model_-directly-from-the-smithy-plugins branch from f59b6e3 to a0dd263 Compare July 6, 2022 16:03
@david-perez david-perez enabled auto-merge (squash) July 6, 2022 16:06
@github-actions
Copy link

github-actions bot commented Jul 6, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

@david-perez david-perez merged commit cd05630 into main Jul 6, 2022
@david-perez david-perez deleted the davidpz/use-the-_symbolprovider_-and-_model_-directly-from-the-smithy-plugins branch July 6, 2022 16:48
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

5 participants