You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2020/10/18 06:51:48 UTC
[GitHub] [shardingsphere-elasticjob] TeslaCN commented on a change in pull request #1567: Refactor ElasticJobServiceLoader, remove unused code and use Optional…
TeslaCN commented on a change in pull request #1567:
URL: https://github.com/apache/shardingsphere-elasticjob/pull/1567#discussion_r507024721
##########
File path: elasticjob-infra/elasticjob-infra-common/src/main/java/org/apache/shardingsphere/elasticjob/infra/handler/threadpool/JobExecutorServiceHandlerFactory.java
##########
@@ -42,8 +43,11 @@
*/
public static JobExecutorServiceHandler getHandler(final String type) {
if (Strings.isNullOrEmpty(type)) {
- return ElasticJobServiceLoader.getCachedInstance(JobExecutorServiceHandler.class, DEFAULT_HANDLER);
+ return ElasticJobServiceLoader.getCachedTypedServiceInstance(JobExecutorServiceHandler.class, DEFAULT_HANDLER)
Review comment:
Is it necessary to check if default handler present? Why could this happen?
##########
File path: elasticjob-infra/elasticjob-infra-common/src/main/java/org/apache/shardingsphere/elasticjob/infra/spi/ElasticJobServiceLoader.java
##########
@@ -106,33 +61,42 @@ private static Object newServiceInstance(final Class<?> clazz) {
/**
* Get cached instance.
*
- * @param typedService service type
+ * @param typedServiceInterface typed service interface
* @param type specific service type
* @param <T> specific type of service
* @return cached service instance
*/
- public static <T extends TypedSPI> T getCachedInstance(final Class<T> typedService, final String type) {
- T instance = TYPED_SERVICES.containsKey(typedService) ? (T) TYPED_SERVICES.get(typedService).get(type) : null;
+ public static <T extends TypedSPI> Optional<T> getCachedTypedServiceInstance(final Class<T> typedServiceInterface, final String type) {
+ T instance = TYPED_SERVICES.containsKey(typedServiceInterface) ? (T) TYPED_SERVICES.get(typedServiceInterface).get(type) : null;
Review comment:
Consider making good use of `Optional#map` if you are going to use `Optional`.
##########
File path: elasticjob-infra/elasticjob-infra-common/src/main/java/org/apache/shardingsphere/elasticjob/infra/spi/ElasticJobServiceLoader.java
##########
@@ -106,33 +61,42 @@ private static Object newServiceInstance(final Class<?> clazz) {
/**
* Get cached instance.
*
- * @param typedService service type
+ * @param typedServiceInterface typed service interface
* @param type specific service type
* @param <T> specific type of service
* @return cached service instance
*/
- public static <T extends TypedSPI> T getCachedInstance(final Class<T> typedService, final String type) {
- T instance = TYPED_SERVICES.containsKey(typedService) ? (T) TYPED_SERVICES.get(typedService).get(type) : null;
+ public static <T extends TypedSPI> Optional<T> getCachedTypedServiceInstance(final Class<T> typedServiceInterface, final String type) {
+ T instance = TYPED_SERVICES.containsKey(typedServiceInterface) ? (T) TYPED_SERVICES.get(typedServiceInterface).get(type) : null;
if (null == instance) {
- throw new JobConfigurationException("Cannot find a cached typed service instance by the interface: @" + typedService.getName() + "and type: " + type);
+ return Optional.empty();
}
- return instance;
+ return Optional.of(instance);
}
/**
* New typed instance.
*
- * @param typedService service type
+ * @param typedServiceInterface typed service interface
* @param type specific service type
* @param <T> specific type of service
* @return specific typed service instance
*/
- public static <T extends TypedSPI> T newTypedServiceInstance(final Class<T> typedService, final String type) {
- Class<?> instanceClass = TYPED_SERVICE_CLASSES.containsKey(typedService) ? TYPED_SERVICE_CLASSES.get(typedService).get(type) : null;
+ public static <T extends TypedSPI> Optional<T> newTypedServiceInstance(final Class<T> typedServiceInterface, final String type) {
+ Class<?> instanceClass = TYPED_SERVICE_CLASSES.containsKey(typedServiceInterface) ? TYPED_SERVICE_CLASSES.get(typedServiceInterface).get(type) : null;
Review comment:
Consider making good use of `Optional#map` if you are going to use `Optional`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org