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

Modify algorithm for parameter transformation logic #616

Merged
merged 2 commits into from
Oct 19, 2023

Conversation

valerena
Copy link
Contributor

Issue #, if available: #573

Description of changes:
Change the way that the parameters are transformed between API GW event and the Servlet request to avoid race condition when using parallel processing while using a HashMap object. Adding a ConcurrentHashMap will likely cause too big of a latency increase, so we move the parallel processing to earlier in the algorithm, and keep the rest simpler.

I added a few comments in the code to make it easier to understand.

We can have a broader discussion about this, and I didn't add any extra test cases. (but I wanted to submit this PR before going on vacation for 10 days)

By submitting this pull request

  • I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • I confirm that I've made a best effort attempt to update all relevant documentation.

Copy link
Collaborator

@deki deki left a comment

Choose a reason for hiding this comment

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

Can you please add a testcase? Looking at the code it's hard to understand so we should avoid it gets broken next time someone touches it...

}
}
}

return new String[0];
return new ArrayList<String>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Collections.emptyList()

@llax87prog
Copy link

thanks for the fix, I did local build and got positive results. I can confirm fix worked for us.

Total request : 157469

Failure rate without fix : 4%
Failure rate with fix : None

@deki deki merged commit c15f931 into aws:main Oct 19, 2023
4 checks passed
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

4 participants