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

Remove unneeded weak reference from prefix logger #22460

Merged
merged 3 commits into from Jun 10, 2017

Conversation

Projects
None yet
5 participants
@jasontedor
Copy link
Member

commented Jan 6, 2017

We have a custom logger implementation known as a prefix logger that is used to write every message by the logger with a given prefix. This is useful for node-level, index-level, and shard-level messages where we want to log the node name, index name, and shard ID, respectively, if possible. The mechanism that we employ is that of a marker. Log4j has a built-in facility for managing these markers, but its effectively a memory leak because these markers are held in a map and can never be released. This is problematic for us since indices and shards do not necessarily have infinite life spans and so on a node where there are many indices being creted and destroyed, this infinite lifespan can be a problem indeed. To solve this, we use our own cache of markers. This is necessary to prevent too many instances of the marker for the same prefix from being created (just think of all the shard-level components that exist in the system), and to workaround the effective leak in Log4j. These markers are stored as weak references in a weak hash map. It is these weak references that are unneeded. When a key is removed from a weak hash map, the corresponding entry is placed on a reference queue that is eventually cleared. This commit simplifies prefix logger by removing this unnecessary weak reference wrapper.

Relates #20429

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2017

For the reviewer, we have a test for the prefix logger functionality in EvilLoggerTests#testPrefixLogger.

@jasontedor jasontedor force-pushed the jasontedor:simplify-prefix-logger branch Jan 6, 2017

Remove unneeded weak reference from prefix logger
We have a custom logger implementation known as a prefix logger that is
used to write every message by the logger with a given prefix. This is
useful for node-level, index-level, and shard-level messages where we
want to log the node name, index name, and shard ID, respectively, if
possible. The mechanism that we employ is that of a marker. Log4j has a
built-in facility for managing these markers, but its effectively a
memory leak because these markers are held in a map and can never be
released. This is problematic for us since indices and shards do not
necessarily have infinite life spans and so on a node where there are
many indices being creted and destroyed, this infinite lifespan can be a
problem indeed. To solve this, we use our own cache of markers. This is
necessary to prevent too many instances of the marker for the same
prefix from being created (just think of all the shard-level components
that exist in the system), and to workaround the effective leak in
Log4j. These markers are stored as weak references in a weak hash
map. It is these weak references that are unneeded. When a key is
removed from a weak hash map, the corresponding entry is placed on a
reference queue that is eventually cleared. This commit simplifies
prefix logger by removing this unnecessary weak reference wrapper.

@jasontedor jasontedor force-pushed the jasontedor:simplify-prefix-logger branch to c6cc6a4 Jan 6, 2017

@rjernst

rjernst approved these changes Jan 6, 2017

Copy link
Member

left a comment

LGTM

public void testPrefixLoggerMarkersCanBeCollected() throws IOException, UserException {
setupLogging("prefix");

for (int i = 0; i < 1 << 20; i++) {

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 6, 2017

Member

Maybe move the limit out to a local, with a comment on the reason for the size (I presume to ensure System.gc() actually collects something below).

@clintongormley clintongormley added v5.4.0 and removed v5.3.0 labels Feb 7, 2017

@jpountz
Copy link
Contributor

left a comment

LGTM

@rjernst

This comment has been minimized.

Copy link
Member

commented Jun 9, 2017

@jasontedor Can this be merged?

jasontedor added some commits Jun 10, 2017

Merge branch 'master' into simplify-prefix-logger
* master: (1889 commits)
  Test: remove faling test that relies on merge order
  Log checkout so SHA is known
  Add link to community Rust Client (#22897)
  "shard started" should show index and shard ID (#25157)
  await fix testWithRandomException
  Change BWC versions on create index response
  Return the index name on a create index response
  Remove incorrect bwc branch logic from master
  Correctly format arrays in output
  [Test] Extending parsing checks for SearchResponse (#25148)
  Scripting: Change keys for inline/stored scripts to source/id (#25127)
  [Test] Add test for custom requests in High Level Rest Client (#25106)
  nested: In case of a single type the _id field should be added to the nested document instead of _uid field.
  `type` and `id` are lost upon serialization of `Translog.Delete`. (#24586)
  fix highlighting docs
  Fix NPE in token_count datatype with null value (#25046)
  Remove the postings highlighter and make unified the default highlighter choice (#25028)
  [Test] Adding test for parsing SearchShardFailure leniently (#25144)
  Fix typo in shards.asciidoc (#25143)
  List Hibernate Search (#25145)
  ...

@jasontedor jasontedor added v5.5.0 v5.6.0 and removed v5.4.2 labels Jun 10, 2017

@jasontedor jasontedor merged commit 5108fa7 into elastic:master Jun 10, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

jasontedor added a commit that referenced this pull request Jun 10, 2017

Remove unneeded weak reference from prefix logger
We have a custom logger implementation known as a prefix logger that is
used to write every message by the logger with a given prefix. This is
useful for node-level, index-level, and shard-level messages where we
want to log the node name, index name, and shard ID, respectively, if
possible. The mechanism that we employ is that of a marker. Log4j has a
built-in facility for managing these markers, but its effectively a
memory leak because these markers are held in a map and can never be
released. This is problematic for us since indices and shards do not
necessarily have infinite life spans and so on a node where there are
many indices being creted and destroyed, this infinite lifespan can be a
problem indeed. To solve this, we use our own cache of markers. This is
necessary to prevent too many instances of the marker for the same
prefix from being created (just think of all the shard-level components
that exist in the system), and to workaround the effective leak in
Log4j. These markers are stored as weak references in a weak hash
map. It is these weak references that are unneeded. When a key is
removed from a weak hash map, the corresponding entry is placed on a
reference queue that is eventually cleared. This commit simplifies
prefix logger by removing this unnecessary weak reference wrapper.

Relates #22460

jasontedor added a commit that referenced this pull request Jun 10, 2017

Remove unneeded weak reference from prefix logger
We have a custom logger implementation known as a prefix logger that is
used to write every message by the logger with a given prefix. This is
useful for node-level, index-level, and shard-level messages where we
want to log the node name, index name, and shard ID, respectively, if
possible. The mechanism that we employ is that of a marker. Log4j has a
built-in facility for managing these markers, but its effectively a
memory leak because these markers are held in a map and can never be
released. This is problematic for us since indices and shards do not
necessarily have infinite life spans and so on a node where there are
many indices being creted and destroyed, this infinite lifespan can be a
problem indeed. To solve this, we use our own cache of markers. This is
necessary to prevent too many instances of the marker for the same
prefix from being created (just think of all the shard-level components
that exist in the system), and to workaround the effective leak in
Log4j. These markers are stored as weak references in a weak hash
map. It is these weak references that are unneeded. When a key is
removed from a weak hash map, the corresponding entry is placed on a
reference queue that is eventually cleared. This commit simplifies
prefix logger by removing this unnecessary weak reference wrapper.

Relates #22460

@jasontedor jasontedor deleted the jasontedor:simplify-prefix-logger branch Jun 10, 2017

@colings86 colings86 added v6.0.0-beta1 and removed v6.0.0 labels Jul 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.