-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: Add Intel Enhanced Optimizations for RDS MySQL Example #522
feat: Add Intel Enhanced Optimizations for RDS MySQL Example #522
Conversation
Consolidated the readme's into one file. Also made comments on main.tf to specify Intel notes.
thank you for the PR - however, I don't know that this is the appropriate place for this. From what I can tell, this is mostly just using the module that you all have created for the parameter group. The examples we have provided here are demonstrations of the various configurations that this module supports - users are absolutely able to provide their own parameter groups and/or parameter group values, but it doesn't really have any bearing on the examples we've provided. Closing this for now |
Hi Bryant, in our example we are showcasing the Intel Optimizations based on our Intel Xeon Tuning Guides. Our goal was not to show how to utilize parameter groups, but to highlight the specific optimizations/configuration that can increase RDS performance based on Intel Architecture. We spoke with @antonbabenko at HashiConf & then again at AWS re:Invent about showcasing these value add to customers. Please let me know if there are additional questions or if a quick meeting would be best. Thank you Josh H cc: @lucasmelogithub |
@bryantbiggs What you say was also my point to Josh when I met him in October. :) Such examples are not as helpful since users can specify whatever they want in the argument. We know that, and usually, we don't accept such contributions. On the other hand, let's consider some changes and review it once more if it makes sense to have it here or not. I recommend that the author make this example 100% self-contained without reference to external modules (like this) and unnecessary logos/images. Include the correct/optimal values for the parameter group as arguments to the module created by this repository - this can be the only interesting part of the example that differentiates it from other similar examples we already have. Leave links to the external repositories/sources, if necessary. @bryantbiggs What do you think? |
I haven't had great experiences with external parties providing contributions that point back to something they have created. Take EKS Blueprints for example - one of the major changes in the refactoring of that projects addons was due to partners not providing the level of support they had originally claimed and we cannot be experts of all the things. So when someone comes to our project and sees something - they will open an issue or raise questions here and we are not equipped to take on all of those requests (both from a resource perspective, but more importantly, from an expertise perspective). And likewise, having these in actual code that is part of the CI test and validation process can quickly lead to problems (i.e. - externally linked code starts failing, which starts failing all of the CI checks here which is outside of our control). However, I do agree that there is value in information sharing and I am sure there are users of this module that would be interested in this type of information. My proposal would be to not have it as functional code as part of the CI tests, but instead static code example that clearly states all questions/issues should be raised on the external project for its related components. For reference, this is what we do now for partner addons in EKS Blueprints https://aws-ia.github.io/terraform-aws-eks-blueprints-addons/main/aws-partner-addons/ We could create a separate docs section like https://github.com/terraform-aws-modules/terraform-aws-alb/blob/master/docs/patterns.md and have a code example and explanation on it there with any relevant links. So its more of a demonstration of how these other modules can further help users of this module but its not something we are actively testing and validating as part of this module. Thoughts? |
I like referring to text documents with or without the code inside. I also don't want us to maintain code where we depend on external repositories we don't control. @lucasmelogithub @JoshHilliker What do you think? Will you be able to do the change? |
@antonbabenko @bryantbiggs - Lucas & I were talking, what are your thoughts about moving the Intel MySQL parameter group module into this repo, that way it's managed under the same tree, but we can still use the sub modular approach. |
I looked into the I don't see a need to make a separate submodule in our repository just for this variable, but instead, I prefer to add a small example or just a markdown file with such values listed. Everyone interested in knowing I will let @bryantbiggs decide if he is open to "a small opinionated example" or a "markdown file" (as Bryant mentioned earlier). I am more in favor of a solution that requires less maintenance (=markdown file). |
I think what is important as well is the benchmarking and branding of Intel behind these data points in the variables.tf. We kept this as a separate module to have updates/consumption via git tags w/ the standard tf source version control. I think having this a separate module with reference to Intel (we can remove image branding) would be of high value to consumers of the module. We plan to continue to update the module as new Xeon generations land on AWS. @antonbabenko @bryantbiggs @lucasmelogithub |
@bryantbiggs @antonbabenko - Happy New Year! Love to pick this back up and get your feedback on my latest comment. Thank You |
Hi @JoshHilliker - so if you wish to keep the module separate and as a standalone module, then I think that we are discussing here is just a markdown file that demonstrates how to use the module with this module plus any other details might need to be aware of and links back to your module/docs. |
Lucas & I talked more. Here's our proposal: 1) removal of Intel branding from Enhanced-Optimization example 2) pull in all parameter group configuration into the stand alone example, so no external dependencies. What do you think? |
|
But we (Anton and I, and other contributors) do not have knowledge or experience with these Intel enhanced optimizations - so it doesn't make sense to have code here for that because we will be unable to troubleshoot it. This is why the static markdown is recommended - we can inform users of these configurations and of the module that will do this for them, but we're not testing and validating that |
Okay.. here's my proposal. we can create an example directory for this and put in the markdown file. good? |
@JoshHilliker Yes, please do that, and link to such an example from the main README.md file in the root. |
Closing PR. A new one will be created per discussion above. |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Description
The PR adds an Intel Enhanced Optimizations for RDS MySQL example. The example implements:
Please squash commits if approved/merged.
Motivation and Context
New Example
Breaking Changes
No
How Has This Been Tested?
examples/*
to demonstrate and validate my change(s)examples/*
projectspre-commit run -a
on my pull requestSuccessfully tested deployment: