Enforce ordering when ObjectProvider is used #18333
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
There was a problem hiding this comment.
Thanks very much for the PR, @ttddyy. I've left a few comments for your consideration when you have a moment.
| @@ -62,9 +62,9 @@ public SpringApplicationAdminMXBeanRegistrar springApplicationAdminRegistrar( | |||
| ObjectProvider<MBeanExporter> mbeanExporters, Environment environment) throws MalformedObjectNameException { | |||
| String jmxName = environment.getProperty(JMX_NAME_PROPERTY, DEFAULT_JMX_NAME); | |||
| if (mbeanExporters != null) { // Make sure to not register that MBean twice | |||
| for (MBeanExporter mbeanExporter : mbeanExporters) { | |||
| mbeanExporters.orderedStream().forEach(mbeanExporter -> { | |||
There was a problem hiding this comment.
I don't think the ordering is important here.
| @@ -123,7 +123,7 @@ public Flyway flyway(FlywayProperties properties, DataSourceProperties dataSourc | |||
| configureCallbacks(configuration, orderedCallbacks); | |||
| fluentConfigurationCustomizers.orderedStream().forEach((customizer) -> customizer.customize(configuration)); | |||
| configureFlywayCallbacks(configuration, orderedCallbacks); | |||
| List<JavaMigration> migrations = javaMigrations.stream().collect(Collectors.toList()); | |||
| List<JavaMigration> migrations = javaMigrations.orderedStream().collect(Collectors.toList()); | |||
There was a problem hiding this comment.
The ordering doesn't matter as Flyway orders them itself based on MigrationVersion returned from getVersion().
| @@ -68,7 +68,7 @@ | |||
| @Autowired | |||
| void bindDataSourcesToRegistry(Map<String, DataSource> dataSources, MeterRegistry registry, | |||
| ObjectProvider<DataSourcePoolMetadataProvider> metadataProviders) { | |||
| List<DataSourcePoolMetadataProvider> metadataProvidersList = metadataProviders.stream() | |||
| List<DataSourcePoolMetadataProvider> metadataProvidersList = metadataProviders.orderedStream() | |||
There was a problem hiding this comment.
Does ordering matter here? The providers are used in a first wins manner. For the ordering to be important there would have to be overlap between providers.
| @@ -67,7 +68,7 @@ public TaskExecutorBuilder taskExecutorBuilder(TaskExecutionProperties propertie | |||
| builder = builder.awaitTermination(shutdown.isAwaitTermination()); | |||
| builder = builder.awaitTerminationPeriod(shutdown.getAwaitTerminationPeriod()); | |||
| builder = builder.threadNamePrefix(properties.getThreadNamePrefix()); | |||
| builder = builder.customizers(taskExecutorCustomizers); | |||
| builder = builder.customizers(taskExecutorCustomizers.orderedStream().collect(Collectors.toList())); | |||
philwebb
added
the
status: waiting-for-feedback
Enforce ordering of customizer in "TaskExecutionAutoConfiguration". Also, add comments on non ordered usage of ObjectProvider.
ttddyy
force-pushed
the
ojectprovider-ordering
branch
from
0c031c1
to
8eecfa1
Compare
|
@wilkinsona |
spring-projects-issues
added
status: feedback-provided
|
@ttddyy thanks for the update but I am not big fan of the comments you've added. We usually don't add those and I don't think it's warranted. I've flagged this one to see what the rest of the team thinks. |
snicoll
added
the
for: team-attention
|
Yeah, we'll drop those when we merge it |
philwebb
added
type: bug
Use an ordered stream in `TaskExecutionAutoConfiguration` when obtaining the TaskExecutor customizers. See gh-18333
|
Thanks for the fix @ttddyy, I've dropped the comments but and merged to 2.1 and 2.2. I also used a method reference trick to save needing to call a collector. The code now looks like this: builder = builder.customizers(this.taskExecutorCustomizers.orderedStream()::iterator);Thanks again! |
Fix to enforce ordering when
ObjectProvideris used for retrieving multiple objects.