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.

Sign up for GitHub

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

Jump to bottom

Lazy initialize deprecation logger in parser #26210

Merged
merged 2 commits into from Aug 15, 2017
Merged

Lazy initialize deprecation logger in parser #26210

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f5ffe7
Lazy initialize deprecation logger in parser
jasontedor Aug 14, 2017
476e641
Merge branch '5.6' into static-deprecation-logger
jasontedor Aug 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 27 additions & 2 deletions core/src/main/java/org/elasticsearch/common/xcontent/support/AbstractXContentParser.java
View file
Open in desktop
Expand Up @@ -20,6 +20,7 @@
package org.elasticsearch.common.xcontent.support;

import org.apache.lucene.util.BytesRef;
import org.apache.lucene.util.SetOnce;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.Booleans;
import org.elasticsearch.common.logging.DeprecationLogger;
Expand Down Expand Up @@ -54,7 +55,31 @@ private static void checkCoerceString(boolean coerce, Class<? extends Number> cl
}
}

private static final DeprecationLogger deprecationLogger = new DeprecationLogger(Loggers.getLogger(AbstractXContentParser.class));
// do not use this field directly, use AbstractXContentParser#getDeprecationLogger
private static final SetOnce<DeprecationLogger> deprecationLogger = new SetOnce<>();

private static DeprecationLogger getDeprecationLogger() {
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason we didn't use the holder idiom here?

ie:

private static class Holder {
  static final DeprecationLogger deprecationLogger = new DeprecationLogger(Loggers.getLogger(AbstractXContentParser.class));
}

private static DeprecationLogger getDeprecationLogger() {
  return Holder.deprecationLogger;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

We can do that too, it's better, I'll open a PR later.

Copy link
Contributor

Choose a reason for hiding this comment

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

++ thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #26218.

/*
* This implementation is intentionally verbose to make the minimum number of volatile reads. In the case that the set once is
* already initialized, this implementation makes exactly one volatile read. In the case that the set once is not initialized we
* make exactly two volatile reads.
*/
final DeprecationLogger logger = deprecationLogger.get();
if (logger == null) {
synchronized (AbstractXContentParser.class) {
final DeprecationLogger innerLogger = deprecationLogger.get();
if (innerLogger == null) {
final DeprecationLogger newLogger = new DeprecationLogger(Loggers.getLogger(AbstractXContentParser.class));
deprecationLogger.set(newLogger);
return newLogger;
} else {
return innerLogger;
}
}
} else {
return logger;
}
}

private final NamedXContentRegistry xContentRegistry;

Expand Down Expand Up @@ -112,7 +137,7 @@ public boolean booleanValue() throws IOException {
booleanValue = doBooleanValue();
}
if (interpretedAsLenient) {
deprecationLogger.deprecated("Expected a boolean [true/false] for property [{}] but got [{}]", currentName(), rawValue);
getDeprecationLogger().deprecated("Expected a boolean [true/false] for property [{}] but got [{}]", currentName(), rawValue);
}
return booleanValue;

Expand Down