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

Logging: Use settings when building daemon threads #32751

Merged
merged 3 commits into from
Aug 20, 2018

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Aug 9, 2018

Subclasses of EsIntegTestCase run multiple Elasticsearch nodes in the
same JVM and when we log we look at the name of the thread to figure out
the node name. This makes sure that all calls to daemonThreadFactory
include the node name.

Closes #32574

I'd like to follow this up with more drastic changes that make it
impossible to do this incorrectly but that change is much larger than
this and I'd like to get these log lines fixed up sooner rather than
later.

Subclasses of `EsIntegTestCase` run multiple Elasticsearch nodes in the
same JVM and when we log we look at the name of the thread to figure out
the node name. This makes sure that all calls to `daemonThreadFactory`
include the node name.

Closes elastic#32574

I'd like to follow this up with more drastic changes that make it
impossible to do this incorrectly but that change is much larger than
this and I'd like to get these log lines fixed up sooner rather than
later.
@nik9000 nik9000 added review :Core/Infra/Logging Log management and logging utilities v7.0.0 v6.5.0 labels Aug 9, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@@ -47,6 +47,7 @@

public class RollupJobTaskTests extends ESTestCase {

private static final Settings SETTINGS = Settings.builder().put("node_name", "test").build();
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the name of this setting node.name instead of node_name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Checking!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I've got it wrong. I'll fix.

Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

LGTM

@nik9000 nik9000 merged commit 462e91d into elastic:master Aug 20, 2018
jasontedor added a commit that referenced this pull request Aug 20, 2018
* master:
  Generalize remote license checker (#32971)
  Trim translog when safe commit advanced (#32967)
  Fix an inaccuracy in the dynamic templates documentation. (#32890)
  Logging: Use settings when building daemon threads (#32751)
  All Translog inner closes should happen after tragedy exception is set (#32674)
  HLREST: AwaitsFix ML Test
  Pass DiscoveryNode to initiateChannel (#32958)
  Add mzn and dz to unsupported locales (#32957)
  Use settings from the context in BootstrapChecks (#32908)
  Update docs for node specifications (#30468)
  HLRC: Forbid all Elasticsearch logging infra (#32784)
  Only configure publishing if it's applied externally (#32351)
  Fixes libs:dissect when in eclipse
  Protect ScriptedMetricIT test cases against failures on 0-doc shards (#32959) (#32968)
  [Kerberos] Add documentation for Kerberos realm (#32662)
  Watcher: Properly find next valid date in cron expressions (#32734)
  Fix some small issues in the getting started docs (#30346)
  Set forbidden APIs target compatibility to compiler java version   (#32935)
  Move connection listener to ConnectionManager (#32956)
nik9000 added a commit that referenced this pull request Aug 20, 2018
Subclasses of `EsIntegTestCase` run multiple Elasticsearch nodes in the
same JVM and when we log we look at the name of the thread to figure out
the node name. This makes sure that all calls to `daemonThreadFactory`
include the node name.

Closes #32574

I'd like to follow this up with more drastic changes that make it
impossible to do this incorrectly but that change is much larger than
this and I'd like to get these log lines fixed up sooner rather than
later.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Aug 21, 2018
…e-types

* elastic/master: (89 commits)
  Fix assertion in AbstractSimpleTransportTestCase (elastic#32991)
  [DOC] Splits role mapping APIs into separate pages (elastic#32797)
  HLRC: ML Close Job (elastic#32943)
  Generalize remote license checker (elastic#32971)
  Trim translog when safe commit advanced (elastic#32967)
  Fix an inaccuracy in the dynamic templates documentation. (elastic#32890)
  Logging: Use settings when building daemon threads (elastic#32751)
  All Translog inner closes should happen after tragedy exception is set (elastic#32674)
  HLREST: AwaitsFix ML Test
  Pass DiscoveryNode to initiateChannel (elastic#32958)
  Add mzn and dz to unsupported locales (elastic#32957)
  Use settings from the context in BootstrapChecks (elastic#32908)
  Update docs for node specifications (elastic#30468)
  HLRC: Forbid all Elasticsearch logging infra (elastic#32784)
  Only configure publishing if it's applied externally (elastic#32351)
  Fixes libs:dissect when in eclipse
  Protect ScriptedMetricIT test cases against failures on 0-doc shards (elastic#32959) (elastic#32968)
  [Kerberos] Add documentation for Kerberos realm (elastic#32662)
  Watcher: Properly find next valid date in cron expressions (elastic#32734)
  Fix some small issues in the getting started docs (elastic#30346)
  ...
@jasontedor
Copy link
Member

@nik9000 I backported this to 6.4 too so that I could get a clean cherry-pick for #32998.

jasontedor pushed a commit that referenced this pull request Aug 21, 2018
Subclasses of `EsIntegTestCase` run multiple Elasticsearch nodes in the
same JVM and when we log we look at the name of the thread to figure out
the node name. This makes sure that all calls to `daemonThreadFactory`
include the node name.

Closes #32574

I'd like to follow this up with more drastic changes that make it
impossible to do this incorrectly but that change is much larger than
this and I'd like to get these log lines fixed up sooner rather than
later.
@nik9000
Copy link
Member Author

nik9000 commented Aug 21, 2018

@nik9000 I backported this to 6.4 too so that I could get a clean cherry-pick for #32998.

Thanks!

@colings86 colings86 added the >test Issues or PRs that are addressing/adding tests label Oct 25, 2018
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Logging Log management and logging utilities >test Issues or PRs that are addressing/adding tests v6.4.1 v6.5.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-node integration tests do not log node IDs correctly
6 participants