mreutegg commented on a change in pull request #4: URL: https://github.com/apache/sling-org-apache-sling-discovery-commons/pull/4#discussion_r707353308
########## File path: src/main/java/org/apache/sling/discovery/commons/providers/util/LogSilencer.java ########## @@ -0,0 +1,97 @@ +/* + * 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.sling.discovery.commons.providers.util; + +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.TimeUnit; + +import org.slf4j.Logger; + +/** + * Helper class to help reduce log.info output. It will avoid repetitive + * log.info calls and instead log future occurrences to log.debug + */ +public class LogSilencer { + + private static final long DEFAULT_AUTO_RESET_DELAY_MINUTES = 10; + + private final Logger logger; + + private final Object syncObj = new Object(); + + private final long autoResetDelayMillis; + + private Map<String, String> lastMsgPerCategory; + + private long autoResetTime = 0; + + public LogSilencer(Logger logger, long autoResetDelaySeconds) { + this.logger = logger; + if (autoResetDelaySeconds > 0) { + autoResetDelayMillis = TimeUnit.MINUTES.toMillis(autoResetDelaySeconds); Review comment: The second parameter should be renamed to `autoResetDelayMinutes` ```suggestion public LogSilencer(Logger logger, long autoResetDelayMinutes) { this.logger = logger; if (autoResetDelayMinutes > 0) { autoResetDelayMillis = TimeUnit.MINUTES.toMillis(autoResetDelayMinutes); ``` ########## File path: src/main/java/org/apache/sling/discovery/commons/providers/base/ViewStateManagerImpl.java ########## @@ -590,6 +595,46 @@ public void run() { } } + protected boolean equalsIgnoreSyncToken(BaseTopologyView newView) { + if (previousView==null) { Review comment: General question on concurrency. Public methods use `lock` to synchronize access to internals. Why is this not necessary here? ########## File path: src/main/java/org/apache/sling/discovery/commons/providers/base/ViewStateManagerImpl.java ########## @@ -590,6 +595,46 @@ public void run() { } } + protected boolean equalsIgnoreSyncToken(BaseTopologyView newView) { + if (previousView==null) { + return false; + } + if (newView==null) { + throw new IllegalArgumentException("newView must not be null"); + } + final ClusterView cluster = newView.getLocalInstance().getClusterView(); + if (cluster instanceof LocalClusterView) { + final LocalClusterView local = (LocalClusterView)cluster; + if (!local.hasPartiallyStartedInstances()) { + // then we should not ignore the syncToken I'm afraid + return previousView.equals(newView); + } + } + final Set<InstanceDescription> previousInstances = previousView.getInstances(); + final Set<InstanceDescription> newInstances = newView.getInstances(); + if (previousInstances.size() != newInstances.size()) { + return false; + } + for (Iterator<InstanceDescription> it = previousInstances.iterator(); it.hasNext();) { + InstanceDescription instanceDescription = (InstanceDescription) it + .next(); + boolean found = false; + for (Iterator<?> it2 = newInstances.iterator(); it2 + .hasNext();) { + InstanceDescription otherId = (InstanceDescription) it2 + .next(); + if (instanceDescription.equals(otherId)) { + found = true; + break; + } + } + if (!found) { + return false; + } + } + return true; Review comment: Can this be replaced with `previousInstances.equals(newInstances)`? ########## File path: src/main/java/org/apache/sling/discovery/commons/providers/spi/LocalClusterView.java ########## @@ -33,4 +38,21 @@ public String getLocalClusterSyncTokenId() { return localClusterSyncTokenId; } + public void setPartiallyStartedClusterNodeIds(Collection<Integer> clusterNodeIds) { Review comment: There are no unit tests for these new methods. I'll create a PR for your feature branch. ########## File path: src/test/java/org/apache/sling/discovery/commons/providers/DummyTopologyView.java ########## @@ -199,7 +200,13 @@ public static DummyTopologyView clone(final DummyTopologyView view) { String clusterId = id.getClusterView().getId(); DefaultClusterView cluster = clusters.get(clusterId); if (cluster==null) { - cluster = new DefaultClusterView(clusterId); + final ClusterView origCluster = id.getClusterView(); + if (origCluster instanceof LocalClusterView) { + final LocalClusterView localOrigCluster = (LocalClusterView) origCluster; + cluster = new LocalClusterView(origCluster.getId(), localOrigCluster.getLocalClusterSyncTokenId()); Review comment: Not sure if this is relevant, but this does not copy over the `partiallyStartedClusterNodeIds`. ########## File path: src/main/java/org/apache/sling/discovery/commons/providers/util/LogSilencer.java ########## @@ -0,0 +1,97 @@ +/* + * 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.sling.discovery.commons.providers.util; + +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.TimeUnit; + +import org.slf4j.Logger; + +/** + * Helper class to help reduce log.info output. It will avoid repetitive + * log.info calls and instead log future occurrences to log.debug + */ +public class LogSilencer { + + private static final long DEFAULT_AUTO_RESET_DELAY_MINUTES = 10; + + private final Logger logger; + + private final Object syncObj = new Object(); + + private final long autoResetDelayMillis; + + private Map<String, String> lastMsgPerCategory; + + private long autoResetTime = 0; + + public LogSilencer(Logger logger, long autoResetDelaySeconds) { + this.logger = logger; + if (autoResetDelaySeconds > 0) { + autoResetDelayMillis = TimeUnit.MINUTES.toMillis(autoResetDelaySeconds); + } else { + autoResetDelayMillis = 0; + } + } + + public LogSilencer(Logger logger) { + this(logger, DEFAULT_AUTO_RESET_DELAY_MINUTES); + } + + public void infoOrDebug(String category, String msg) { + final boolean doLogInfo; + synchronized (syncObj) { + if (autoResetTime == 0 || System.currentTimeMillis() > autoResetTime) { + reset(); + } + if (lastMsgPerCategory == null) { + lastMsgPerCategory = new HashMap<>(); + } + final String localLastMsg = lastMsgPerCategory.get(category); + if (localLastMsg == null || !localLastMsg.equals(msg)) { + doLogInfo = true; + lastMsgPerCategory.put(category, msg); + } else { + doLogInfo = false; + } + } + if (doLogInfo) { + logger.info(msg + " (future identical logs go to debug)"); Review comment: Minor improvement. ```suggestion logger.info("{} {}", msg, "(future identical logs go to debug)"); ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
