add DgsDataLoaderOptionsCustomizer

[lmy654]

Pull Request type

• Bugfix
• Feature
• Refactoring (no functional changes, no api changes)
• Build related changes
• Other (please describe):

Changes in this PR

Be able to cusomize more about a DataLoader's DataLoaderOptions, #201

[srinivasankavitha]

Could you also provide an example of what the DgsDataLoaderCustomizer would look like? Also, the tests don't actually verify that the custom options are set. Is there a way to test for that?

[lmy654]

Could you also provide an example of what the DgsDataLoaderCustomizer would look like? Also, the tests don't actually verify that the custom options are set. Is there a way to test for that?

Yeah, actually in our system, we hava an demand to customize the implementation of org.dataloader.CacheMap and org.dataloader.CacheKey and config DataLoaderOptions with them like below:

public class HotKeyCacheMap<U, V> implements CacheMap<U, V> {

    private Map<U, V> cache;

    private HotKeyCache hotKeyCache;

    /**
     * Default constructor
     */
    public HotKeyCacheMap(HotKeyCache hotKeyCache) {
        cache = new HashMap<>();
        this.hotKeyCache = hotKeyCache;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public boolean containsKey(U key) {
        return cache.containsKey(key) || hotKeyCacheContainsKey(key);
    }

    private boolean hotKeyCacheContainsKey(U key){
        ValueModel valueModel = hotKeyCache.getValueModel(key.toString());
        return  valueModel != null && !valueModel.isDefaultValue();
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public V get(U key) {
        if(cache.containsKey(key)) {
            return cache.get(key);
        }
        else {
            return (V)hotKeyCache.getValueModel(key.toString()).getValue();
        }
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public CacheMap<U, V> set(U key, V value) {
        cache.put(key, value);
        hotKeyCache.setHotValue(key.toString(), value);
        return this;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public CacheMap<U, V> delete(U key) {
        cache.remove(key);
        return this;
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public CacheMap<U, V> clear() {
        cache.clear();
        return this;
    }
}

@Configuration
public class HotkeyConfiguration {

    @Bean
    public DgsDataLoaderOptionsCustomizer dgsDataLoaderOptionsCustomizer(HotKeyCacheMapService hotKeyCacheMapService){
        return (dgsDataLoader, dataLoaderOptions) -> {
            dataLoaderOptions.setCacheMap(hotKeyCacheMapService.getCacheMap());
            dataLoaderOptions.setCacheKeyFunction(input -> dgsDataLoader.name() + "-" + input);
        };
    }
}

HotKeyCacheMapService#getCacheMap() would return a instance of HotKeyCacheMap. The HotKeyCache is a second level data cache to store some really "hot" data to protect downstream service from being overwhelmed, and we also need to customize the org.dataloader.CacheKey to distinct the key in different DataLoaders.

About the tests, I will provide a specific implementation to verify that custom options.

[srinivasankavitha]

The changes are looking good to me.

[srinivasankavitha]

@paulbakker @berngp - could you also take a look?

[srinivasankavitha]

Tested these changes and it works well.

[paulbakker]

@lmy654 thanks for working on this.

It looks like the mechanism is designed to have all dataloaders be configured in the same way. Is that correct? I'm wondering if it's common to have different configurations, eg. different caching strategies, for different dataloaders. I guess you could still do that based on the dataloader reference passed in into the customizer.

As an alternative, would something like a @DgsDataLoaderOptionsCustomizer(MyCustomizer.class) make sense?

Just bouncing ideas to get an understanding of what the best approach would be.

[lmy654]

@lmy654 thanks for working on this.

It looks like the mechanism is designed to have all dataloaders be configured in the same way. Is that correct? I'm wondering if it's common to have different configurations, eg. different caching strategies, for different dataloaders. I guess you could still do that based on the dataloader reference passed in into the customizer.

As an alternative, would something like a @DgsDataLoaderOptionsCustomizer(MyCustomizer.class) make sense?

Just bouncing ideas to get an understanding of what the best approach would be.

@paulbakker Thanks for review. I understand your concern. Before I thought configurations like cacheMap, cacheKeyFuntion should be common for dataloaders in most scenarios, whereas other configurations like batchingEnabled, maxBatchSize are not common and have been customized by DgsDataLoader's properties. Now I think again, as a framework, we should always give user an option. Although now it could be done based on the dataloader reference passed in into the customizer, it is not good way.

As @DgsDataLoader has already some properties for customizing DataLoaderOptions, I think we could add another property specifing the bean name of DgsDataLoaderOptionsCustomizer and don't need to add another annotation. What do you think about?
I have just tried to refactor this, like below:
dgs-framework code:

@Target({ElementType.TYPE, ElementType.FIELD})
@Retention(RetentionPolicy.RUNTIME)
@Component
public @interface DgsDataLoader {
    String name();
    boolean caching() default true;
    boolean batching() default true;
    int maxBatchSize() default 0;
    String optionsCustomizerName() default "";
}

class DgsDataLoaderProvider(private val applicationContext: ApplicationContext) {
    
    // omit some code...

    private fun createDataLoader(
        batchLoader: BatchLoader<*, *>,
        dgsDataLoader: DgsDataLoader
    ): DataLoader<out Any, out Any>? {
        val options = DataLoaderOptions.newOptions()
            .setBatchingEnabled(dgsDataLoader.batching)
            .setCachingEnabled(dgsDataLoader.caching)
        if (dgsDataLoader.maxBatchSize > 0) {
            options.setMaxBatchSize(dgsDataLoader.maxBatchSize)
        }
        if (dgsDataLoader.optionsCustomizerName.isNotBlank()) {
            applicationContext.getBean(dgsDataLoader.optionsCustomizerName, DgsDataLoaderOptionsCustomizer::class.java)
                .customize(dgsDataLoader, options)
        }
        val extendedBatchLoader = wrappedDataLoader(batchLoader, dgsDataLoader.name)
        return DataLoader.newDataLoader(extendedBatchLoader, options)
    }

    // omit some code
}

application code:

@Configuration
public class ExampleConfiguration {
    @Bean
    public DgsDataLoaderOptionsCustomizer exampleDgsDataLoaderOptionsCustomizer
(HotKeyCacheMapService hotKeyCacheMapService){
        return (dgsDataLoader, dataLoaderOptions) -> {
            dataLoaderOptions.setCacheMap(hotKeyCacheMapService.getCacheMap());
            dataLoaderOptions.setCacheKeyFunction(input -> dgsDataLoader.name() + "-" + input);
        };
    }
}

@DgsDataLoader(name = "exampleLoader", optionsCustomizerName = "exampleDgsDataLoaderOptionsCustomizer")
class ExampleBatchLoader : BatchLoader<String, String> {
    override fun load(keys: MutableList<String>?): CompletionStage<MutableList<String>> {
        return CompletableFuture.supplyAsync { mutableListOf("a", "b", "c") }
    }
}

I have already tested on my test environment, and pushed updated code.

[paulbakker]

@lmy654 I think specifying it in the annotation is a good way. I'm wondering if we can use a Class instead of the bean name though. Something like:

public @interface DgsDataLoader {
    String name();
    boolean caching() default true;
    boolean batching() default true;
    int maxBatchSize() default 0;
    Class<?> optionsCustomizer() default Object.class;
}

WDYT?

[lmy654]

@paulbakker I think it might be hard to use a Class sometimes if we defined a bean using lamba expression or an anonymous inner class, e.g. like this:

@Configuration
public class ExampleConfiguration {
    @Bean
    public DgsDataLoaderOptionsCustomizer exampleDgsDataLoaderOptionsCustomizer1
(HotKeyCacheMapService hotKeyCacheMapService){
        return (dgsDataLoader, dataLoaderOptions) -> {
            dataLoaderOptions.setCacheMap(hotKeyCacheMapService.getCacheMap());
        };
    }

    @Bean
    public DgsDataLoaderOptionsCustomizer exampleDgsDataLoaderOptionsCustomizer2() {
        return (dgsDataLoader, dataLoaderOptions) -> {
            dataLoaderOptions.setCacheKeyFunction(input -> dgsDataLoader.name() + "-" + input);
        };
    }
}

@DgsDataLoader(name = "exampleLoader1", optionsCustomizerName = "exampleDgsDataLoaderOptionsCustomizer1")
class ExampleBatchLoader1 : BatchLoader<String, String> {
    override fun load(keys: MutableList<String>?): CompletionStage<MutableList<String>> {
        return CompletableFuture.supplyAsync { mutableListOf("a", "b", "c") }
    }
}
@DgsDataLoader(name = "exampleLoader2", optionsCustomizerName = "exampleDgsDataLoaderOptionsCustomizer2")
class ExampleBatchLoader2 : BatchLoader<String, String> {
    override fun load(keys: MutableList<String>?): CompletionStage<MutableList<String>> {
        return CompletableFuture.supplyAsync { mutableListOf("d", "e", "f") }
    }
}

[prokop7]

Any progress on this?

[evanmalmud]

Anything stopping this from being merged than the conflicts?
Would love to have this feature to be able to use a CacheMap/ValueCache.

[prokop7]

#1485
This pull request partially solves the problems.

Would love to have this feature to be able to use a CacheMap/ValueCache.
You definitely can use DataLoaderOptionsProvider to utilize custom cachemap/valuecache