This repository has been archived by the owner on Aug 2, 2022. It is now read-only.
Use the correct ctor for NodeDetailsCollector #166
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sruti1312
approved these changes
Aug 5, 2020
sidheart
approved these changes
Aug 5, 2020
khushbr
approved these changes
Aug 5, 2020
vigyasharma
reviewed
Aug 5, 2020
| final ConfigOverrides newOverrides = ConfigOverridesHelper.deserialize(newSettingValue); | ||
| overridesHolder.setCurrentClusterConfigOverrides(newOverrides); | ||
| overridesHolder.setLastUpdatedTimestamp(System.currentTimeMillis()); | ||
| if (newSettingValue != null && !newSettingValue.isEmpty()) { |
There was a problem hiding this comment.
I will add a UT for this.
| @@ -188,7 +188,7 @@ public PerformanceAnalyzerPlugin(final Settings settings, final java.nio.file.Pa | |||
|
|
|||
| scheduledMetricCollectorsExecutor.addScheduledMetricCollector(new MetricsPurgeActivity()); | |||
|
|
|||
| scheduledMetricCollectorsExecutor.addScheduledMetricCollector(new NodeDetailsCollector()); | |||
There was a problem hiding this comment.
Can we remove this default constructor from NodeDetailsCollector, or is it required?
There was a problem hiding this comment.
Let me check the usages. Maybe the no arg constructor is not used anymore.
sruti1312
added a commit
that referenced
this pull request
Aug 24, 2020
* Fix NullPointerException when PA starts collecting metrics before master node is fully up * Fix unit tests on Mac. Fix NPE during MasterServiceEventMetrics collection. * Reorder imports, refactor unit tests * add RFC for RCA * Create README.md * Split Elasticsearch version independent code (#75) This commit has 3 major changes - 1) Performance Analyzer code that is Elasticsearch version independent. 3) Performance optimization in the Elasticsearch plugin to emit events to a single event log file. This brings down CPU utilization by an order of magnitude on large clusters. * Update Performance Analyzer to support ElasticSearch version 7.3.2 * This commit merges some of the fixes and features that should have been in the split version of PA. Features and fixes introduced in this PR: Allow performance analyzer to be en(dis)abled through a cluster setting across the cluster. Allow logging to be controllable through the cluster setting Capture a node's role along with the host address and the node name Checkstyle compliance Some issues that are still not addressed in this PR: Update build scripts to start the agent from the reader location instead of the plugin location Remove pa_config, pa_bin and other folders that are already present in the reader * Adding the dependency on the renamed jar performanceanalyzer-rca from performanceanalyzer * Delete Unused test class Remove NewFormatProcessorTest class which is not used. * Adding shardsPerCollection REST API to update the shards Per Collection in node stats collector (#83) * Update gradle wrapper * Add isMasterNode to NodeDetailsStatus (#84) * make the unit test backward compatible with the isMasterNode in NodeDetailsStatus * Create gradle.yml (#87) * Create gradle.yml * Update gradle.yml * Update gradle.yml * Added the bouncy castle jars * Added the licenses file * Add cd.yml and enable CD pipeline to upload artifact to S3 (#90) * add cd.yml * upgrade ospackage version to 8.2.0 * change s3 * update cd.yml * removing the -i flag * fix a bug in StatsTests.java (#97) * Update CONTRIBUTORS.md * Update CONTRIBUTORS.md * We must handle all exceptions while intercepting ES requests (#99) * Making sure that we don't throw exceptions while intercepting ES requests PerformanceAnalyzer intercepts various ES request paths toget detailed metrics. But today if we throw an exception, then it will bubble all the way upto ES and fail the request. * Addressing the PR comments * Updating the .gitignore * style changes * Adding Shard Size Metric as a part of Node Stats (#101) * Adding Shard Size Metric as a part of Node Stats * removing the -i flag * fix a bug in StatsTests.java (#97) * Update CONTRIBUTORS.md * Update CONTRIBUTORS.md * Adressing Typos Co-authored-by: Aditya Jindal <[email protected]> Co-authored-by: Joydeep Sinha <[email protected]> Co-authored-by: Ruizhen Guo <[email protected]> Co-authored-by: Balaji <[email protected]> * collect queue latency metric in PerformanceAnalyzer (#111) Authored-By: rguo-aws * Remove unnecessary string formatting (#112) * Odfe it framework release (#107) * ODFE IT Framework POC * Testing to see if Dockerstuff is set up * Modified workflow to set DOCKER_COMPOSE_LOCATION * Modify workflow to include stacktrace and no symbolic linkage * Set DOCKER_COMPOSE_LOCATION using set-env * Try set-env in a different location * Attempt to fix docker-compose set-env * Make workflow set vm.max_map_count * Use sudo when setting vm.max_map_count * Make performance-analyzer execute integration tests on checkin * Clean up PerformanceAnalyzerIT and build.gradle script * Add newline to end of gradle.properties file * Modify gradle.yml and checkMetrics * Fix ObjectMapper allocation and move TestUtils definition Co-authored-by: Sid Narayan <[email protected]> * Add github badges (#114) * Add github badges * Add github badges * Run integ tests as part of git workflow instead of build (#115) This commit makes it so that you can build PA without running integration tests. This is useful for many reasons, including being able to build RCA without depending on PA's integration tests and dramatically reducing build times. Integration Testing has been added as part of the Github Actions workflow * Fixup ITs and binding issues (#119) This commit makes IT execution much more robust and only executes ITs if the user passes the -Dtests.enableIT flag to the Gradle environment. This commit also ensures that we bind to all interfaces when we spin up a local Docker cluster for testing. * collect queue capacity on writer (#118) * Pa build fix (#122) * Remove * junit import * Fix logger usage * Ignore JsonKeyTests * Remove sed operation from build.gradle The sed logic is now baked into the Dockerfile in performance-analyzer-rca so it's no longer necessary here. * Restore JsonKeyTests * PA will no longer crash when SecurityManager says no (#113) * PA will no longer crash when SecurityManager says no PA attempts to set the default SSL Socket Factory (which defines rules for the SSL Sockets it creates) as well as default hostname verification rules when it is initialized by the Elasticsearch plugin loader. However, this behavior would result in an AccessControlException when run alongside the opendistro-security plugin. This commit is a simple fix which allows these two plugins to work together. * Update logging to WARN level * Calculate rejection increase and emit the delta increase of rejection as metric (#124) * Enable spotbugs, address spotbug warnings (#126) * Fix cluster state when pa is enabled from controller (#125) * Fix cluster state when pa is Enabled from controller * Add license info * Move PA files to subdir owned by elasticsearch user (#146) * IT improvements (#143) * Use true/false instead of null/present for integTest props integTest is a gradle task which runs our integration tests. It uses system properties like -Dtests.useDockerCluster to decide whether or not to perform certain actions like spinning up a docker cluster for testing. The task would previously perform the property's action if the property was present. This commit makes the integTest task only execute a system property action if that property is set to "true" * Make IT port number configurable The PerformanceAnalyzerIT class previously assumed that the Performance Analyzer Webservice would always be listening on port 9600 for any deployment of PerformanceAnalyzer. Since this isn't always the case, this commit makes the port number configurable through a gradle property. * Allow logging to be enabled for ensurePaAndRcaEnabled * cache max size metric collector (#145) * Adding changes to collect Cache Max Size metric * Updating the Cache Max Size Dimension to use toString (#153) * Fixing checkstyle build failure (#158) * Add an IT which verifies that the RCA REST endpoint can be queried (#157) * Add an IT which verifies that the RCA REST endpoint can be queried * Add try-catch to handle 404 exceptions * Add initial support for dynamic config overriding (#148) * Add initial support for dynamic config overriding * Use helper to serialize/deserialize instead of the wrapper * Add licence header to new files * Update licence year to 2020 * Node collector split (#162) Node Collector split is created based on the metrics which are required for all the shards on the node and other which can be collected on a few number of shards per iteration. Built the Jar from this Patch and applied on the AES cluster. The Cache related metrics which should be collected for all the shards irrespective of the value of shardsPerCollection value are getting collected. Tested with a zero value of this parameter (shardsPerCollection). * Use the correct ctor for NodeDetailsCollector (#166) * Use the correct ctor for NodeDetailsCollector * Check for null ConfigOverrides wrapper while appending timestamps * Add unit test for null cluster setting (#167) * Use the correct ctor for NodeDetailsCollector * Check for null ConfigOverrides wrapper while appending timestamps * Add unit test for null cluster setting for config overrides * Split capacity/latency collecting logic into separate try/catch block (#168) * Update PULL_REQUEST_TEMPLATE.md * Fix invalid cluster state (#172) * Fix invalid cluster state * Address PR comments * Skip RCA tests when building PA in Github workflows (#177) * Build against elasticsearch 7.9 and resolve dependency conflicts * Add licenses for dependencies * Add licenses for dependencies * Change minor version * Add release notes and contributors * Changed licenses * Modify github workflows * Fix merge conflicts * Fix jarHell around log4j Co-authored-by: Karthik Kumarguru <[email protected]> Co-authored-by: Karthik Kumarguru <[email protected]> Co-authored-by: Partha Kanuparthy <[email protected]> Co-authored-by: Partha Kanuparthy <[email protected]> Co-authored-by: Adithya Chandra <[email protected]> Co-authored-by: Venkata Jyothsna Donapati <[email protected]> Co-authored-by: Palash Hedau <[email protected]> Co-authored-by: Joydeep Sinha <[email protected]> Co-authored-by: Balaji <[email protected]> Co-authored-by: khushbr <[email protected]> Co-authored-by: Chandra <[email protected]> Co-authored-by: Pardeep Singh <[email protected]> Co-authored-by: Ruizhen <[email protected]> Co-authored-by: Joydeep Sinha <[email protected]> Co-authored-by: Joydeep Sinha <[email protected]> Co-authored-by: Ruizhen Guo <[email protected]> Co-authored-by: Aditya Jindal <[email protected]> Co-authored-by: Aditya Jindal <[email protected]> Co-authored-by: Ricardo L. Stephen <[email protected]> Co-authored-by: Sid Narayan <[email protected]> Co-authored-by: Sid Narayan <[email protected]> Co-authored-by: Peter Zhu <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
Fixes #165
Fixes #164
Description of changes:
The NodeDetailsCollector instantiation in the plugin was overwritten by change #162
This change instantiates the node details collector correctly.
This change also gets rid of the confusing stacktrace that gets printed during ES startup.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.