Closed Emily-Jiang closed 3 years ago
Minutes
@ConfigProperties(prefix="server.")
syntax. Emily explained that they are leaning towards dropping the period and possibly introducing a delimiter property. (Editor view -- definitely drop the period, but introducing a delimiter property sounds like overkill.)@Inject
. He was questioning whether this was accidentally defining behavior for CDI vs Config... Tom will talk with Emily offline.@Inject
-- whether the presence or absence of @Inject
implies the use of a CDI managed bean or not... On that same chart, Emily indicated that the @ApplicationScoped
should actually be @DependentScoped
. Part 2 item.Part 2 of the Config 2.0 UFO Review was held on Monday, Aug 24, 2020. Link to recording: https://ibm.box.com/s/z0bkm80s8srutrw36ji6mbk1ab3rpx53 Link to presentation: https://ibm.ent.box.com/file/658940841177
The only comment was from Emily - she plans to update chart 14 ("To Be" for application properties defined in server.xml) to be clearer about how the value of a variable like 'key1' defined at the server level but getting its value from the microprofile-config.properties could have different values (or no value) depending on the contents of that properties file for different applications.
Beta blog #14519
I've addressed the comments from @kwsutter and @follis and uploaded the deck https://ibm.ent.box.com/file/658940841177
@NottyCode all of the above links lead me to the Open Liberty Design Documents folder. Not sure why you could not get there. Try this link: https://ibm.box.com/s/uxvsj5lgob9ffpqzxjg6b1kkopiltp1z
Serviceability Approval Comment - Please answer the following questions for serviceability approval:
WAD -- does the WAD identify the most likely problems customers will see and identify how the feature will enable them to diagnose and solve those problems without resorting to raising a PMR? Have these issues been addressed in the implementation?
Test and Demo -- As part of the serviceability process we're asking feature teams to test and analyze common problem paths for serviceability and demo those problem paths to someone not involved in the development of the feature (eg. L2, test team, or another development team).
a) What problem paths were tested and demonstrated?
b) Who did you demo to?
c) Do the people you demo'd to agree that the serviceability of the demonstrated problem scenarios is sufficient to avoid PMRs for any problems customers are likely to encounter, or that L2 should be able to quickly address those problems without need to engage L3?
SVT -- SVT team is often the first team to try new features and often encounters problems setting up and using them. Note that we're not expecting SVT to do full serviceability testing -- just to sign-off on the serviceability of the problem paths they encountered. a) Who conducted SVT tests for this feature? b) Do they agree that the serviceability of the problems they encountered is sufficient to avoid PMRs, or that L2 should be able to quickly address those problems without need to engage L3?
Which L2 / L3 queues will handle PMRs for this feature? Ensure they are present in the contact reference file and in the queue contact summary, and that the respective L2/L3 teams know they are supporting it. Ask Don Bourne if you need links or more info.
Does this feature add any new metrics or emit any new JSON events? If yes, have you updated the JMX metrics reference list / Metrics reference list / JSON log events reference list in the Open Liberty docs?
Issue raised for docs (for the ID section): https://github.com/OpenLiberty/docs/issues/3443
Could we have a performance review for this please @jdmcclur ?
(FYI: https://github.com/OpenLiberty/open-liberty/pull/15117)
Perf looks pretty good. I am ok approving it. There are still some things we could work on in the future, some I have mentioned before in 15117.
Serviceability Approval Comment - Please answer the following questions for serviceability approval:
Since mpConfig-2.0 moves to using SmallRye Config's implementation of MicroProfile Config, the servicability of the whole feature needs to be considered, rather than just this feature version.
A plethora of exception tests exist in the TCKs, though these only test the Exception
type that was thrown, and not the Error message
. Our FAT tests which observe the error messages are here: Config20ExceptionTests.java.
- WAD -- does the WAD identify the most likely problems customers will see and identify how the feature will enable them to diagnose and solve those problems without resorting to raising a PMR? Have these issues been addressed in the implementation?
The most likely problems/mistakes that a customer could make are:
Attempting to retrieve a Config Property which doesn't exist a. Programatically
config.getValue("nonExistingKey", String.class);
b. By Injection
@Inject
@ConfigProperty(name = "nonExistingKey")
String nonExistingKeyValue;
Attempting to retrieve a Config Property to a Type without a Converter a. Programatically
config.getValue("existingKeyValue", TypeWithoutConverter.class);
b. By Injection
@Inject
@ConfigProperty(name = "existingKey")
TypeWithoutConverter existingKeyValue;
Attempting to Inject a Config Property without defining the name field (and the resolved Config Value not being present)
@Inject
@ConfigProperty
String nonExistingKey;
Attempting to convert an invalid Config Value, e.g., where invaldValue=,
exists as a Config Property
a. Programatically
config.getValue("invaldValue", String[].class);
b. By Injection
@Inject
@ConfigProperty(name = "invaldValue")
String[] invaldValue;
(mpConfig-2.0) Defining a @ConfigProperties
class which uses an invalid prefix. Where invaldPrefix.akey
doesn't exist, and ConfigPropertiesPOJO.java
:
@ConfigProperties(prefix = "invaldPrefix")
@Dependent
public class ConfigPropertiesPOJO {
public String akey;
}
(mpConfig-2.0) Accessing a valid @ConfigProperties
class defining a new invalid prefix. Where validPrefix.akey
does exist and invalidPrefix.akey
doesn't exist, and ConfigPropertiesPOJO.java
:
@ConfigProperties(prefix = "validPrefix")
@Dependent
public class ConfigPropertiesPOJO {
public String akey;
}
a. Programatically
CDI.current().select(ConfigPropertiesPOJO.class, ConfigProperties.Literal.of("invaldPrefix")).get();
b. By Injection
@Inject
@ConfigProperties(prefix = "invaldPrefix")
ConfigPropertiesPOJO details;
(mpConfig-2.0) Attempting to get a ConfigValue
object for a Config Property which doesn't exist (mpConfig-2.0)
a. Programatically
ConfigValue configValueProg = config.getConfigValue("nonExistingKey");
b. By Injection
@Inject
@ConfigProperty(name = "nonExistingKey")
private ConfigValue configValueInj;
The feature enables customers to diagnose these problems by having a comprehensive set of logging message, mainly covered in ConfigMessages.java and InjectionMessages.java. Respectively:
a. Programatically: java.util.NoSuchElementException: SRCFG00014: Property nonExistingKey not found
b. By Injection: DeploymentException: SRCFG02000: No Config Value exists for required property nonExistingKey
a. Programatically: IllegalArgumentException: SRCFG00013: No Converter registered for class io.openliberty.guides.config.TypeWithoutConverter
b. By Injection: DeploymentException: SRCFG02006: The property existingKey cannot be converted to class io.openliberty.guides.config.TypeWithoutConverter
DeploymentException: SRCFG02000: No Config Value exists for required property <class_name>.nonExistingKey
(JavaDoc: where class_name is the fully qualified name of the class being injected to)
a. Programatically: NoSuchElementException: SRCFG00014: Property invaldValue not found
b. By Injection: DeploymentException: SRCFG02004: Required property invaldValue not found
NoSuchElementException: SRCFG00014: Property invalidPrefix.akey not found
(note: this has been updated: https://github.com/smallrye/smallrye-config/commit/7821dc058eb7e6af7b18675810129fce27b6608f)
a. Programatically: NoSuchElementException: SRCFG00028: Could not find a mapping for <class_name>.ConfigPropertiesPOJO with prefix invaldPrefix
b. By Injection: NoSuchElementException: SRCFG00014: Property invaldPrefix.akey not found
a. Programatically: configValueProg.getValue() -> null
b. By Injection: configValueInj.getValue() -> null
(Correct behaviour according to JavaDoc)
- Test and Demo -- As part of the serviceability process we're asking feature teams to test and analyze common problem paths for serviceability and demo those problem paths to someone not involved in the development of the feature (eg. L2, test team, or another development team). a) What problem paths were tested and demonstrated?
All of the 7 most likely problem cases were demonstrated.
Tests for 2 and 3 exist here in our FATs: Config20ExceptionTests.java#L95 and Config20ExceptionTests.java#L111 respectively.
Tests for 1, 4, 5, and 7 exist here in the TCKs: ConfigProviderTest.java#L122, EmptyValuesTestProgrammaticLookup.java#L96, ConfigPropertiesMissingPropertyInjectionTest.java#L61, and ConfigValueTest.java#L69 respectively.
I could write a test for 6?
b) Who did you demo to?
@scottcurtis2605
c) Do the people you demo'd to agree that the serviceability of the demonstrated problem scenarios is sufficient to avoid PMRs for any problems customers are likely to encounter, or that L2 should be able to quickly address those problems without need to engage L3?
a. Programatically: sufficient b. By Injection: sufficient
a. Programatically: sufficient b. By Injection: sufficient - but perhaps should mention that a converter doesn't exist
sufficient
Both could mention that it's an invalid conversion (according to the conversion rules here), rather than just saying "It's missing". This is however quite pedantic, and is quite a rare edge case.
Sufficient, but perhaps it could mention something specific to Config Properties, for example, "No Config Property exists for the prefix <prefix name>
for the field <field name>
". Note: this has already been improved: https://github.com/smallrye/smallrye-config/commit/7821dc058eb7e6af7b18675810129fce27b6608f
a. Programatically: sufficient b. By Injection: Same as 5
a. Programatically: correct behaviour b. By Injection: correct behaviour
- SVT -- SVT team is often the first team to try new features and often encounters problems setting up and using them. Note that we're not expecting SVT to do full serviceability testing -- just to sign-off on the serviceability of the problem paths they encountered. a) Who conducted SVT tests for this feature? b) Do they agree that the serviceability of the problems they encountered is sufficient to avoid PMRs, or that L2 should be able to quickly address those problems without need to engage L3?
@hanczaryk Please could you look at this?
- Which L2 / L3 queues will handle PMRs for this feature? Ensure they are present in the contact reference file and in the queue contact summary, and that the respective L2/L3 teams know they are supporting it. Ask Don Bourne if you need links or more info.
WL3-CDI
- Does this feature add any new metrics or emit any new JSON events? If yes, have you updated the JMX metrics reference list / Metrics reference list / JSON log events reference list in the Open Liberty docs?
NA
L2 has requested STE slides for this feature. The STE template can be found at the links below. You can use either one to create the education.
Slide Template: https://ibm.box.com/s/1an42g7zdgmaj84w7dft0indqfgi8ffm
Github Template: https://pages.github.ibm.com/WASL3/site/STE/about
Please upload the completed slides to the same 'STE Archive' BOX folder or provide me the Github link. Thanks!
Please upload the completed slides to the same 'STE Archive' BOX folder or provide me the Github link. Thanks!
Thank you @skasund. I have uploaded the file STE-2021-MP-Config2.0.ppt
to the STE Archive
folder
@Joseph-Cass Thanks for the STE slides
@Joseph-Cass , thanks for the thorough comments above on serviceability. Were you able to address comments from @scottcurtis2605? One concern I have on the exception messages is they don't all mention that this relates to config. For example "No Converter registered for class" relies on an administrator to realize that the message ID means that you're talking about MP Config.
@Joseph-Cass , thanks for the thorough comments above on serviceability. Were you able to address comments from @scottcurtis2605? One concern I have on the exception messages is they don't all mention that this relates to config. For example "No Converter registered for class" relies on an administrator to realize that the message ID means that you're talking about MP Config.
@donbourne I was intending on raising an issue in SmallRye Config, and if they agree, pulling in any changes in the next release. Since the suggested changes were minor. What do you think?
I do agree, I think it would be better if we could somehow include that information, though I don't know the feasibility of it. I'll have a look. Thanks!
@Joseph-Cass , given they are messages on an exception, which means they will generally be accompanied by a stack trace that will give context on what action triggered the failure, I think it's ok to get this into next release (and not hold up this release). Can you please paste the link to the issue you're raising here with SR to close this out?
Can you please paste the link to the issue you're raising here with SR to close this out?
I agree. Thanks @donbourne . Here's the issue: https://github.com/smallrye/smallrye-config/issues/485
Thanks @Joseph-Cass , looks good. I'll wait for confirmation from @hanczaryk to make sure SVT is happy with serviceability before approving.
Here are the SVT answers for https://github.com/OpenLiberty/open-liberty/issues/11016#issuecomment-744484449
a) Who conducted SVT tests for this feature?
Brian Hanczaryk
b) Do they agree that the serviceability of the problems they encountered is sufficient to avoid PMRs, or that L2 should be able to quickly address those problems without need to engage L3? Yes, I agree that the serviceability of any problems encountered is sufficient to avoid PMRs, or that L2 should be able to quickly address those problems without the need to engage L3.
Joseph and I discussed the info the he provided in #3443. From our discussion, he added specific examples. The information that he provide looks good. Approving.
In the upcoming MicroProfile Config release 2.0, we are going to introduce backward incompatible changes to update some API methods and change some behaviours such as null or empty debate.
Instructions:
Design
Before Development Starts or 8 weeks before Onboarding
Beta
If your feature, or portions of it, are going to be included in a beta
Before Onboarding the beta
kind=beta
,ibm:beta
,ProductInfo.getBetaEdition()
)1 week before beta GA
Legal
3 weeks before Onboarding
Translation
3 weeks before Onboarding
Feature Complete
2 weeks before Onboarding
Focal Point Approvals
2 to 1 week before Onboarding
You MUST have the Design Approved or No Design Approved label before requesting focal point approvals.
All features (both "Design Approved" and "No Design Approved")
"Design Approved" features
Ready for GA
1 week before Onboarding
1 week before GA
Other deliverbles