-
Notifications
You must be signed in to change notification settings - Fork 135
IGNITE-17358 Metric exporters' configurations. #1098
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
Conversation
b83ddd1 to
fbd3f4a
Compare
agura
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please. fix some minors.
...rc/main/java/org/apache/ignite/internal/metrics/configuration/MetricConfigurationModule.java
Outdated
Show resolved
Hide resolved
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/MetricManager.java
Outdated
Show resolved
Hide resolved
...org/apache/ignite/internal/metrics/exporters/TestPullMetricsExporterConfigurationSchema.java
Outdated
Show resolved
Hide resolved
...org/apache/ignite/internal/metrics/exporters/TestPullMetricsExporterConfigurationSchema.java
Outdated
Show resolved
Hide resolved
...org/apache/ignite/internal/metrics/exporters/TestPushMetricsExporterConfigurationSchema.java
Outdated
Show resolved
Hide resolved
...org/apache/ignite/internal/metrics/exporters/TestPushMetricsExporterConfigurationSchema.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/ignite/internal/metrics/configuration/MetricConfigurationModule.java
Outdated
Show resolved
Hide resolved
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/MetricManager.java
Outdated
Show resolved
Hide resolved
| private List<MetricExporter> metricExporters; | ||
| private Map<String, MetricExporter> availableExporters; | ||
|
|
||
| private MetricConfiguration metricConfiguration; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my understanding, at least this field should be volatile, because of we cannot guarantee that configure() and start() methods are called in the same thread or properly synchronized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
configure and start has happens-before relations by design and must be called in sequence. Otherwise, we can start MetricManager with null configuration. In real code - IgniteImpl will do it
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/MetricManager.java
Outdated
Show resolved
Hide resolved
...rationTest/java/org/apache/ignite/internal/metrics/exporters/MetricExportersLoadingTest.java
Outdated
Show resolved
Hide resolved
...s/metrics/src/main/java/org/apache/ignite/internal/metrics/exporters/PushMetricExporter.java
Show resolved
Hide resolved
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/MetricManager.java
Show resolved
Hide resolved
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/MetricManager.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Slava Koptilin <slava.koptilin@gmail.com>
…ernal/metrics/exporters/TestPullMetricsExporterConfigurationSchema.java Co-authored-by: Slava Koptilin <slava.koptilin@gmail.com>
Signed-off-by: Slava Koptilin <slava.koptilin@gmail.com>
Signed-off-by: Slava Koptilin <slava.koptilin@gmail.com>
No description provided.