Skip to content
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

Use Spring's ApplicationContext to instantiate exporter arguments #7628

Closed
npepinpe opened this issue Aug 11, 2021 · 2 comments · Fixed by #9854
Closed

Use Spring's ApplicationContext to instantiate exporter arguments #7628

npepinpe opened this issue Aug 11, 2021 · 2 comments · Fixed by #9854
Assignees
Labels
area/ux Marks an issue as related to improving the user experience kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. scope/broker Marks an issue or PR to appear in the broker section of the changelog scope/gateway Marks an issue or PR to appear in the gateway section of the changelog version:8.1.0-alpha4 version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0

Comments

@npepinpe
Copy link
Member

Description

One of the biggest downside with exporters is that arguments are parsed as an intermediate map by Spring. We then use Gson to instantiate a bean from that map. That's primarily due to historical reasons, where our configuration used to be TOML and parsed by Gson - at the time it made sense to simply reuse this. However, what happens now is that we have differences between how the normal Zeebe configuration is parsed and how exporter configuration is instantiated.

The goal here is to align this, such that there is no difference, and the exporter configuration is parsed like any Spring Boot configuration. Since we cannot guarantee that these classes are annotated correctly - and we don't want to enforce it to avoid coupling user code to Spring Boot - we can use an adapter for the ApplicationContext which will let us dynamically register beans at run time, and then instantiate them correctly.

As mentioned, this must be done in two phases. Spring can only instantiate beans from a set of definitions. At startup, it scans the class path (or whatever you defined) and loads the bean definitions via annotations. If your class is not annotated, or wasn't scanned, then it won't know it, and cannot instantiate anything from it. Once a bean definition (i.e. a class and a name) is registered, then it can be easily instantiated without any issue.

See this example:

package io.camunda.zeebe.shared;

import java.util.HashSet;
import java.util.Set;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.support.GenericBeanDefinition;
import org.springframework.context.support.GenericApplicationContext;
import org.springframework.stereotype.Component;

@Component
public final class SpringBeanFactory {
  @Autowired private final GenericApplicationContext context;

  private final Set<String> registeredBeans = new HashSet<>();

  public SpringBeanFactory(final GenericApplicationContext context) {
    this.context = context;
  }

  public <T> T createBean(final String prefix, final Class<T> type) {
    if (!registeredBeans.contains(prefix)) {
      registerBeanDefinition(prefix, type);
    }

    return context.getBean(prefix, type);
  }

  private synchronized void registerBeanDefinition(final String prefix, final Class<?> type) {
    final var definition = new GenericBeanDefinition();
    definition.setBeanClass(type);
    context.registerBeanDefinition(prefix, definition);
    registeredBeans.add(prefix);
  }
}

Using this, you can then call springBeanFactory.createBean("zeebe.broker", BrokerCfg.class) which will instantiate the configuration by looking up the property "zeebe.broker". You don't even need to annotate the BrokerCfg class, which would also let us remove the Spring dependency on the broker module.

@npepinpe npepinpe added kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. scope/broker Marks an issue or PR to appear in the broker section of the changelog Impact: Usability scope/gateway Marks an issue or PR to appear in the gateway section of the changelog blocker/stakeholder Marks an issue as blocked, waiting for stakeholder input labels Aug 11, 2021
@npepinpe npepinpe removed the blocker/stakeholder Marks an issue as blocked, waiting for stakeholder input label Sep 8, 2021
@npepinpe npepinpe added area/ux Marks an issue as related to improving the user experience and removed Impact: Usability labels Apr 11, 2022
@Zelldon
Copy link
Member

Zelldon commented Jul 4, 2022

This is a really annoying bug, not only toil which we need to get away.

I run now multiple times into this (I guess others as well). Recently again with @romansmirnov it took me a while to find the workaround again so I will post it here.


If you want to configure an exporter argument, like for the elastic search exporter the numberOfShards currently doesn't work via an environment variable. You need to set it as a java argument.

The following example refers to the helm charts, so instead of:

  env:
    - name: ZEEBE_BROKER_EXPORTERS_ELASTICSEARCH_ARGS_INDEX_NUMBEROFSHARDS
      value: "1"

in your values file.

You have to set it as:

  javaOpts: >-
    -XX:MaxRAMPercentage=25.0
    -XX:+ExitOnOutOfMemoryError
    -XX:+HeapDumpOnOutOfMemoryError
    -XX:HeapDumpPath=/usr/local/zeebe/data
    -Dzeebe.broker.exporters.elasticsearch.args.index.numberOfShards=1

See the related SO answer I gave (with help from @npepinpe kudos to you :)) https://stackoverflow.com/a/72056369/2165134

@npepinpe npepinpe self-assigned this Jul 21, 2022
@npepinpe
Copy link
Member Author

I ended up using a simpler fix, though it's not ideal. We can use Jackson to allow relaxed, case insensitive deserialization, which fixes this case. However we're still not deserializing as with Spring, which isn't great as we still have different ways of deserializing configuration.

The issue with the proposed solution is that it doesn't work with nested bean definitions, i.e. you need to walk the class structure and register beans for all types it references. Do-able, but definitely increasing the complexity of the PR.

So I'm tabling this for now.

@Zelldon Zelldon added the version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0 label Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ux Marks an issue as related to improving the user experience kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. scope/broker Marks an issue or PR to appear in the broker section of the changelog scope/gateway Marks an issue or PR to appear in the gateway section of the changelog version:8.1.0-alpha4 version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants