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

Refactor use of ServerConfigurationFactory #1572

Merged
merged 3 commits into from
Apr 8, 2020

Conversation

milleruntime
Copy link
Contributor

@milleruntime milleruntime commented Mar 26, 2020

Divided this PR into 2 commits:
1 - Deprecates the init method in MemoryManager that took ServerConfigurationFactory as a parameter. Replace it with a method that takes ServerContext. Updated Test to mock ServerContext.
2 - Inline some of the methods of ServerConfigurationFactory into ServerContext to reduce references to it. Removed sychronization on the methods now they are a part of ServerContext because i don't think its needed since each server should only have one instance. This is just a first step into eliminating internal use of ServerConfigurationFactory.

Update: Broke out changes to MemoryManager in a separate branch.

Copy link
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the overall changes. I think this might prompt follow-up work, though.

Comment on lines +144 to +147
if (defaultConfig == null) {
defaultConfig = DefaultConfiguration.getInstance();
}
return defaultConfig;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's any reason to load this singleton lazily.

Suggested change
if (defaultConfig == null) {
defaultConfig = DefaultConfiguration.getInstance();
}
return defaultConfig;
return DefaultConfiguration.getInstance();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this method is poorly named because it just returns a new object:

public static DefaultConfiguration getInstance() {
    return new DefaultConfiguration();
  }

I think it is worth storing a local copy to prevent creating too many identical objects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class is documented as a singleton... but that appears to have stopped being the case as of 37f900b ; it should be a singleton. It doesn't need to do all the synchronization that the commit cleaned up, but it should still save and reuse the single instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah and since it anything loading the class will use it, I think it could just be created once during class load as a static variable.

@ctubbsii ctubbsii added this to In progress in 2.1.0 via automation Mar 27, 2020
@milleruntime milleruntime merged commit 58e7087 into apache:master Apr 8, 2020
2.1.0 automation moved this from In progress to Done Apr 8, 2020
@milleruntime milleruntime deleted the serverConfFactory branch April 8, 2020 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
2.1.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants