You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2021/08/24 14:19:18 UTC
[GitHub] [sling-org-apache-sling-discovery-commons] stefan-egli opened a new pull request #4: SLING-10489 : ignore partially started instances :
stefan-egli opened a new pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-discovery-commons/pull/4
a. skipping activeIds in OakBacklogClusterSyncService if they are partially started
b. ignore syncToken for view change checks if there are partially started instances
plus bonus c. LogSilencer introduced, which reduces log.info spam caused by discovery
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-discovery-commons] stefan-egli commented on pull request #4: SLING-10489 : ignore partially started instances :
Posted by GitBox <gi...@apache.org>.
stefan-egli commented on pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-discovery-commons/pull/4#issuecomment-919216953
Thx for the review! merging.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-discovery-commons] mreutegg commented on pull request #4: SLING-10489 : ignore partially started instances :
Posted by GitBox <gi...@apache.org>.
mreutegg commented on pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-discovery-commons/pull/4#issuecomment-918320275
Unit test for LocalClusterView as PR: https://github.com/stefan-egli/sling-org-apache-sling-discovery-commons/pull/2
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-discovery-commons] stefan-egli commented on a change in pull request #4: SLING-10489 : ignore partially started instances :
Posted by GitBox <gi...@apache.org>.
stefan-egli commented on a change in pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-discovery-commons/pull/4#discussion_r708145134
##########
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:
+1, got lost in early changes
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-discovery-commons] stefan-egli commented on a change in pull request #4: SLING-10489 : ignore partially started instances :
Posted by GitBox <gi...@apache.org>.
stefan-egli commented on a change in pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-discovery-commons/pull/4#discussion_r708148009
##########
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:
It might not be relevant as this dummy test class is not used by the new code - but I think it needs to copy it as well, to make it consistent. Will do that.
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-discovery-commons] stefan-egli commented on a change in pull request #4: SLING-10489 : ignore partially started instances :
Posted by GitBox <gi...@apache.org>.
stefan-egli commented on a change in pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-discovery-commons/pull/4#discussion_r708147273
##########
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:
This is a bit convoluted and thus not very user friendly indeed. The use of `lock` in this class is a result of early refactoring to make the class reusable. That arguably makes it a bit awkward, and I would actually prefer if it was done differently. Having said that, the way it is currently, is that the `lock` object is shared between this class and the caller (that's where awkwardness comes in). And `equalsIgnoreSyncToken` is considered a method which only happens within a `lock.lock()`.
I'll add a note to this method's javadoc to make it a bit more explicit and clear
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-discovery-commons] stefan-egli commented on a change in pull request #4: SLING-10489 : ignore partially started instances :
Posted by GitBox <gi...@apache.org>.
stefan-egli commented on a change in pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-discovery-commons/pull/4#discussion_r708145713
##########
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:
indeed, I think that should be possible. I will do that but also add some test cases to be on the safe side (plus it has no unit test coverage anyway)
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-discovery-commons] stefan-egli merged pull request #4: SLING-10489 : ignore partially started instances :
Posted by GitBox <gi...@apache.org>.
stefan-egli merged pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-discovery-commons/pull/4
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-discovery-commons] mreutegg commented on a change in pull request #4: SLING-10489 : ignore partially started instances :
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-discovery-commons] stefan-egli commented on a change in pull request #4: SLING-10489 : ignore partially started instances :
Posted by GitBox <gi...@apache.org>.
stefan-egli commented on a change in pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-discovery-commons/pull/4#discussion_r708148135
##########
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:
thx!
--
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-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org