Skip to content

Generation support for 1.11 - build dev 1.10 pr16 #2060

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

Closed
wants to merge 31 commits into from

Conversation

sdavtaker
Copy link
Contributor

@sdavtaker sdavtaker commented Aug 26, 2022

Description of changes:

  • Updating Generator Java code
  • Adding message about REGENERATE_CLIENT removal
  • Refactor generate_sdks.py
  • Updating Generator templates for 1.10

This change doesn't include regeneration of all clients, only DDB, neither the inclussion of all of them at the generated directory level. Holding that for follow up PR since will create a lot of noise here.

Check all that applies:

  • Did a review by yourself.
  • Added proper tests to cover this PR. (If tests are not applicable, explain.)
  • Checked if this PR is a breaking (APIs have been changed) change.
  • Checked if this PR will not introduce cross-platform inconsistent behavior.
  • Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

  • Linux
  • Windows
  • Android
  • MacOS
  • IOS
  • Other Platforms

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

sdavtaker and others added 30 commits August 19, 2022 11:06
All cmake modules in cmake were moved to cmake_legacy.
Most of the main CMakeLists.txt was moved to cmake_legacy/legacy_main.cmake
Option for disabling LEGACY_BUILD was created. But, no implementation yet.
New src directory contains core and core unit tests
New test directory contains integration tests for selected autogenerated service clients
New tools directory contains tools for code generation, special builds, documentation generation, etc...
prefetch_crt script removed, it will not longer be needed at 1.10
Some scripts moved to tools will be later consolidated in the build-system
This commit is a breaking change but it is too noisy, so making it back to healthy in next commit by only
submitting the real changes to keep things together as before.
Build fails from mixed use of types from CRT and SDK that was
fixed after the creation of this branch.
The rebase is going to be huge, so breaking the push in 2 steps
befor continuing.
Introduced generated exported visibility header.
Added all core-tests to the build for ctest.
All tests build, not all pass, fixes coming up next.
Build and export header changes to use as reference
for the required code-generation changes in following
commits.
Moved Testing-Resources target to be a library since we use
it in all the tests, integ and unit.
Fixed missed inclussion in platform testing that prevented
symbols to be accessible for the consumers of the library.
Added generated Testing-Resources export.
Added build of DynamoDB integration tests.
@sdavtaker sdavtaker force-pushed the build-dev-1.10-pr16 branch from c447f4f to 1204dc7 Compare August 26, 2022 19:38
@sdavtaker sdavtaker marked this pull request as ready for review August 26, 2022 19:41
if(metadata.getSignatureVersion().equals("v4") || metadata.getSignatureVersion().equals("s3v4")) {
return true;
}
return operations.values().parallelStream().anyMatch(operation -> operation.getSignerName().equals("Aws::Auth::SIGV4_SIGNER"));
Copy link
Contributor

Choose a reason for hiding this comment

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

parallel steams are generally advised against in the java world because they use the common fork-join pool, meaning they share an executor pool with the rest of the world. I would either use sequential streams which is often faster. or alternatively it can have its own executor pool.

if(retryableErrors.contains(error.getName())) {
error.setRetryable(true);
}
for (Error error : serviceModel.getServiceErrors()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could be simplified to

error.setRetryable(serviceModel.getServiceErrors()
  .findAny(error -> retryableErrors.contains(error.getName())))

VelocityContext context = createContext(serviceModel);

List<String> files = new ArrayList<>();
for (int i = 0; i < fileList.size(); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could be simplified using streams to

fileList.stream()
  .filter(Objects::nonNull)
  .map(File:: getPathRelativeToRoot)
  .filter(filePath -> filePath.endsWith("cpp") && filePath.contains("/model/")
  .map(filePath -> Paths.get(filePath).getFileName().toString())
  .collect(Collectors.toList())

@@ -123,6 +125,11 @@ public SdkFileEntry[] generateSourceFiles(ServiceModel serviceModel) throws Exce
replicationStatus.getEnumValues().set(indexOfComplete, "COMPLETED");
}

// Some S3 operations have embedded errors, and we need to search for errors in the response.
Copy link
Contributor

Choose a reason for hiding this comment

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

i recognize this code 🙂

@@ -1,152 +1,190 @@
# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
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 a wrong script to modify.
You could just delete 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.

I'm going to do a clean up of all no longer scripts in this directory and merge this back to the right place.
Moving to draft in the mean time.

@sdavtaker sdavtaker marked this pull request as draft August 26, 2022 20:25
@sdavtaker sdavtaker changed the title Generation support for 1.10 - build dev 1.10 pr16 Generation support for 1.11 - build dev 1.10 pr16 Nov 28, 2022
@sdavtaker sdavtaker marked this pull request as ready for review February 15, 2023 18:21
@jmklix jmklix closed this Jan 26, 2024
@jmklix jmklix deleted the build-dev-1.10-pr16 branch January 26, 2024 18:30
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.

4 participants