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

AppConfig instance variables need to be made volatile #2578

Closed
at055612 opened this issue Nov 17, 2021 · 4 comments
Closed

AppConfig instance variables need to be made volatile #2578

at055612 opened this issue Nov 17, 2021 · 4 comments
Assignees
Labels
bug p:normal Normal priority
Projects

Comments

@at055612
Copy link
Member

We need to add volatile to all the props as these config singletons are being read and updated by multiple threads so memory effects are a possibility, though slight, given the frequency of config changes.

Need volatile on all instance variables except those of a type that extends AbstractConfig.

@at055612 at055612 added the bug label Nov 17, 2021
@at055612 at055612 self-assigned this Nov 17, 2021
@at055612 at055612 added this to To Do Bugs in Stroom 7.0 via automation Nov 17, 2021
@at055612 at055612 added the p:normal Normal priority label Nov 17, 2021
@at055612
Copy link
Member Author

at055612 commented Nov 22, 2021

The alternative to this is to change to using immutable config objects.

We will need to change all config classes to have a @JsonCreator ctor, make all props final, remove all setters and add a no args ctor that will create the obj will all the default values. Depending on how we build objects from the yaml we may want to replace all primitives with boxed versions and the full args ctor to replace any nulls with the defaults.

We will need to inject config objects using providers. The providers will use some object that holds a hashmap of <Class, ? extends AppConfig> to get the instance to inject. The hashmap will be populated by:

  1. Walk a vanilla AppConfig tree to establish what all the default values are and set up the globalPropertyMap.
  2. Parse the yaml file (see below) to establish what the yaml overridden values are and to set those values in the globalPropertyMap
  3. Read all the db props and set any db overrides in the globalPropertyMap
  4. Use the globalPropertyMap to build a full object tree using all the effective values, putting each new object in a new class keyed hashmap, then swap out the previous hashmap with the new one.

To parse the sparse yaml file we may be able to do it manually using objectMapper.readTree to walk the sparse file. This way we don't have to worry about jackson's handling of null/default values when mapping to our POJOs. We can just get the value from the yaml file and if it is different to what we know the default to be then set it as a yaml override. If we stick with using mapping to POJOs from the file then we will need to ensure all POJOs use boxed types.

In order to create instances of config objects using the full args ctor we will need to use reflection to look at the @JsonProperty annos on the ctor to establish the ctor arg order so we can call it with all the right prop values.

All classes that inject config may need to be changed to inject a provider instead, depending on whether the class is short lived or not. Also need to change uses of the config to potentially get from the provider first rather than holding an instance of the config on its instance so it is always using up to date config. This does mean each call to get will invoke a hashmap lookup so for some high performance applications holding a local temporary copy may be needed.

@at055612
Copy link
Member Author

Some code is on branch use-boxed-primitives

@at055612
Copy link
Member Author

at055612 commented Nov 24, 2021

This is the preferred pattern for a config class

@JsonPropertyOrder(alphabetic = true)
@JsonInclude(Include.NON_NULL)
public class XConfig extends AbstractConfig {

  @JsonProperty // Optional unless we need a custom name
  private final Integer i;

  // no args ctor relies on creator ctor for defaults
  public XConfig() {
    return this(null);
  }

  // The creator ctor is resposible for setting all defaults
  @JsonCreator
  public XConfig(@JsonProperty("i") final Integer i) {
    this.i = Objects.requireNonNullElse(i, 99);
  }

  public int getI() {
    return i;
  }
  // + equals(), hashcode(), toString()
}

at055612 added a commit that referenced this issue Nov 25, 2021
at055612 added a commit that referenced this issue Nov 30, 2021
at055612 added a commit that referenced this issue Nov 30, 2021
at055612 added a commit that referenced this issue Dec 3, 2021
Split out common parts of YamlUtil.
Refactor proxy config (re)loading
at055612 added a commit that referenced this issue Dec 3, 2021
at055612 added a commit that referenced this issue Dec 3, 2021
at055612 added a commit that referenced this issue Dec 3, 2021
at055612 added a commit that referenced this issue Dec 6, 2021
at055612 added a commit that referenced this issue Dec 6, 2021
at055612 added a commit that referenced this issue Dec 7, 2021
at055612 added a commit that referenced this issue Dec 7, 2021
at055612 added a commit that referenced this issue Dec 7, 2021
at055612 added a commit that referenced this issue Dec 7, 2021
at055612 added a commit that referenced this issue Dec 10, 2021
App now spins up a minimalist injector with just enough binds
to bootstrapp the DB and config. Once that is complete it creates
a child injector with the rest of the app which ensures the injected
config is valid.
at055612 added a commit that referenced this issue Dec 13, 2021
at055612 added a commit that referenced this issue Dec 13, 2021
at055612 added a commit that referenced this issue Dec 13, 2021
at055612 added a commit that referenced this issue Dec 14, 2021
at055612 added a commit that referenced this issue Dec 14, 2021
at055612 added a commit that referenced this issue Dec 14, 2021
@at055612
Copy link
Member Author

Fixed in > beta.166

Stroom 7.0 automation moved this from To Do Bugs to Done Dec 14, 2021
gcdev373 added a commit that referenced this issue Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug p:normal Normal priority
Projects
Stroom 7.0
  
Done
Development

No branches or pull requests

1 participant