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

Peons should not report SysMonitor stats since MiddleManager reports them. #12802

Merged
merged 4 commits into from
Jul 23, 2022
Merged

Peons should not report SysMonitor stats since MiddleManager reports them. #12802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e33bed8
Peons don't load SysMonitor Stats any longer.
tejaswini-imply Jul 19, 2022
5df6216
increases test coverage
tejaswini-imply Jul 19, 2022
a7252b2
introduces NoopSysMonitor to run on peons
tejaswini-imply Jul 20, 2022
5af7e64
adds NoopSysMonitorTest
tejaswini-imply Jul 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions core/src/main/java/org/apache/druid/java/util/metrics/NoopSysMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.apache.druid.java.util.metrics;

import org.apache.druid.java.util.emitter.service.ServiceEmitter;

public class NoopSysMonitor extends SysMonitor
{
public NoopSysMonitor()
{
super();
}

@Override
public boolean doMonitor(ServiceEmitter emitter)
{
return false;
}
}
42 changes: 42 additions & 0 deletions core/src/test/java/org/apache/druid/java/util/metrics/NoopSysMonitorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.apache.druid.java.util.metrics;

import org.apache.druid.java.util.emitter.service.ServiceEmitter;
import org.junit.Assert;
import org.junit.Assume;
import org.junit.Test;
import org.mockito.Mockito;

public class NoopSysMonitorTest
{
private static final String CPU_ARCH = System.getProperty("os.arch");

@Test
public void testDoMonitor()
{
Assume.assumeFalse("aarch64".equals(CPU_ARCH));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think it should be safe for this test to run on all architectures, but I guess there's no harm in skipping this test on aarch64

Suggested change
Assume.assumeFalse("aarch64".equals(CPU_ARCH));

Copy link
Member Author

Choose a reason for hiding this comment

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

This test fails on aarch64 since SysMonitor is the parent of NoopSysMonitor. Sigar still gets loaded even though there's no need for it. Or should there be another class that these two classes inherit from to avoid this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops I didn't notice that when I skimmed the change. The NoopSysMonitor should just implement Monitor since it is doing nothing. Or you can extend AbstractMonitor and return false in the doMonitor() method.

Copy link
Member Author

Choose a reason for hiding this comment

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

But MetricsModule is getting the instance of SysMonitor through Guice while loading, and getSysMonitor() provides for SysMonitor. In case of peon returning NoopSysMonitor here.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok. feel free to ignore my suggestion above.

There is a way to refactor this so that sigar won't initialize on the peons, but it seems like overkill.

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 thought of adding logic in getMonitorScheduler during loading for the same and it seemed overkill. If you don't mind could you please share if there is other way for future references.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we wanted to do something like that, I would have made SysMonitor an interface and make the current class an implementation of that interface.

Another way to approach this would be to add a new annotation and inject the Monitor interface that way https://github.com/google/guice/wiki/BindingAnnotations


ServiceEmitter serviceEmitter = Mockito.mock(ServiceEmitter.class);
NoopSysMonitor noopSysMonitor = new NoopSysMonitor();

Assert.assertFalse(noopSysMonitor.doMonitor(serviceEmitter));
}
}
50 changes: 44 additions & 6 deletions server/src/main/java/org/apache/druid/server/metrics/MetricsModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,15 @@
import com.google.inject.Key;
import com.google.inject.Module;
import com.google.inject.Provides;
import com.google.inject.TypeLiteral;
import com.google.inject.name.Names;
import io.timeandspace.cronscheduler.CronScheduler;
import org.apache.druid.discovery.NodeRole;
import org.apache.druid.guice.DruidBinders;
import org.apache.druid.guice.JsonConfigProvider;
import org.apache.druid.guice.LazySingleton;
import org.apache.druid.guice.ManageLifecycle;
import org.apache.druid.guice.annotations.Self;
import org.apache.druid.java.util.common.IAE;
import org.apache.druid.java.util.common.concurrent.Execs;
import org.apache.druid.java.util.common.logger.Logger;
Expand All @@ -43,9 +46,11 @@
import org.apache.druid.java.util.metrics.JvmThreadsMonitor;
import org.apache.druid.java.util.metrics.Monitor;
import org.apache.druid.java.util.metrics.MonitorScheduler;
import org.apache.druid.java.util.metrics.NoopSysMonitor;
import org.apache.druid.java.util.metrics.SysMonitor;
import org.apache.druid.query.ExecutorServiceMonitor;

import javax.annotation.Nullable;
import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -170,13 +175,46 @@ public JvmThreadsMonitor getJvmThreadsMonitor(DataSourceTaskIdHolder dataSourceT
@Provides
@ManageLifecycle
public SysMonitor getSysMonitor(
DataSourceTaskIdHolder dataSourceTaskIdHolder
DataSourceTaskIdHolder dataSourceTaskIdHolder,
Injector injector
)
{
Map<String, String[]> dimensions = MonitorsConfig.mapOfDatasourceAndTaskID(
dataSourceTaskIdHolder.getDataSource(),
dataSourceTaskIdHolder.getTaskId()
);
return new SysMonitor(dimensions);
final Set<NodeRole> nodeRoles = getNodeRoles(injector);

if (isPeonRole(nodeRoles)) {
return new NoopSysMonitor();
} else {
Map<String, String[]> dimensions = MonitorsConfig.mapOfDatasourceAndTaskID(
dataSourceTaskIdHolder.getDataSource(),
dataSourceTaskIdHolder.getTaskId()
);
return new SysMonitor(dimensions);
}
}

@Nullable
private static Set<NodeRole> getNodeRoles(Injector injector)
{
try {
return injector.getInstance(
Key.get(
new TypeLiteral<Set<NodeRole>>()
{
},
Self.class
)
);
}
catch (Exception e) {
return null;
}
}

private static boolean isPeonRole(Set<NodeRole> nodeRoles)
{
if (nodeRoles == null) {
return false;
}
return nodeRoles.contains(NodeRole.PEON);
}
}
61 changes: 61 additions & 0 deletions server/src/test/java/org/apache/druid/server/metrics/MetricsModuleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,17 @@
package org.apache.druid.server.metrics;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.inject.Binder;
import com.google.inject.CreationException;
import com.google.inject.Guice;
import com.google.inject.Injector;
import com.google.inject.Key;
import com.google.inject.Module;
import com.google.inject.Scopes;
import com.google.inject.TypeLiteral;
import com.google.inject.name.Names;
import org.apache.druid.discovery.NodeRole;
import org.apache.druid.guice.GuiceInjectors;
import org.apache.druid.guice.JsonConfigProvider;
import org.apache.druid.guice.LazySingleton;
Expand All @@ -37,22 +40,31 @@
import org.apache.druid.jackson.JacksonModule;
import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.java.util.emitter.service.ServiceEmitter;
import org.apache.druid.java.util.emitter.service.ServiceEventBuilder;
import org.apache.druid.java.util.metrics.BasicMonitorScheduler;
import org.apache.druid.java.util.metrics.ClockDriftSafeMonitorScheduler;
import org.apache.druid.java.util.metrics.MonitorScheduler;
import org.apache.druid.java.util.metrics.NoopSysMonitor;
import org.apache.druid.java.util.metrics.SysMonitor;
import org.apache.druid.server.DruidNode;
import org.hamcrest.CoreMatchers;
import org.junit.Assert;
import org.junit.Assume;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.mockito.ArgumentMatchers;
import org.mockito.Mockito;

import javax.validation.Validation;
import javax.validation.Validator;
import java.util.Properties;
import java.util.Set;

public class MetricsModuleTest
{
private static final String CPU_ARCH = System.getProperty("os.arch");

@Rule
public ExpectedException expectedException = ExpectedException.none();

Expand Down Expand Up @@ -155,6 +167,55 @@ public void testGetMonitorSchedulerUnknownSchedulerException()
createInjector(properties).getInstance(MonitorScheduler.class);
}

@Test
public void testGetSysMonitorViaInjector()
{
// Do not run the tests on ARM64. Sigar library has no binaries for ARM64
Assume.assumeFalse("aarch64".equals(CPU_ARCH));

final NodeRole nodeRole = NodeRole.PEON;
final Injector injector = Guice.createInjector(
new JacksonModule(),
new LifecycleModule(),
binder -> {
binder.bindScope(LazySingleton.class, Scopes.SINGLETON);
},
binder -> {
binder.bind(
new TypeLiteral<Set<NodeRole>>()
{
}).annotatedWith(Self.class).toInstance(ImmutableSet.of(nodeRole));
}
);
final DataSourceTaskIdHolder dimensionIdHolder = new DataSourceTaskIdHolder();
injector.injectMembers(dimensionIdHolder);
final MetricsModule metricsModule = new MetricsModule();
final SysMonitor sysMonitor = metricsModule.getSysMonitor(dimensionIdHolder, injector);
final ServiceEmitter emitter = Mockito.mock(ServiceEmitter.class);
sysMonitor.doMonitor(emitter);

Assert.assertTrue(sysMonitor instanceof NoopSysMonitor);
Mockito.verify(emitter, Mockito.never()).emit(ArgumentMatchers.any(ServiceEventBuilder.class));
}

@Test
public void testGetSysMonitorWhenNull()
{
// Do not run the tests on ARM64. Sigar library has no binaries for ARM64
Assume.assumeFalse("aarch64".equals(CPU_ARCH));

final Injector injector = createInjector(new Properties());
final DataSourceTaskIdHolder dimensionIdHolder = new DataSourceTaskIdHolder();
injector.injectMembers(dimensionIdHolder);
final MetricsModule metricsModule = new MetricsModule();
final SysMonitor sysMonitor = metricsModule.getSysMonitor(dimensionIdHolder, injector);
final ServiceEmitter emitter = Mockito.mock(ServiceEmitter.class);
sysMonitor.doMonitor(emitter);

Assert.assertFalse(sysMonitor instanceof NoopSysMonitor);
Mockito.verify(emitter, Mockito.atLeastOnce()).emit(ArgumentMatchers.any(ServiceEventBuilder.class));
}

private static Injector createInjector(Properties properties)
{
return Guice.createInjector(
Expand Down