You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2022/10/05 20:03:32 UTC
[GitHub] [activemq-artemis] gtully opened a new pull request, #4241: ARTEMIS-4025 - trap failure to apply properties as errors and report …
gtully opened a new pull request, #4241:
URL: https://github.com/apache/activemq-artemis/pull/4241
…via the broker status in a configuration/properties/errors status field
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a diff in pull request #4241: ARTEMIS-4025 - trap failure to apply properties as errors and report …
Posted by GitBox <gi...@apache.org>.
gtully commented on code in PR #4241:
URL: https://github.com/apache/activemq-artemis/pull/4241#discussion_r989951955
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/impl/ConfigurationImpl.java:
##########
@@ -2852,13 +3038,14 @@ public Configuration setSuppressSessionNotifications(boolean suppressSessionNoti
}
@Override
- public String getStatus() {
- return status;
+ public synchronized String getStatus() {
Review Comment:
sorted
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a diff in pull request #4241: ARTEMIS-4025 - trap failure to apply properties as errors and report …
Posted by GitBox <gi...@apache.org>.
gtully commented on code in PR #4241:
URL: https://github.com/apache/activemq-artemis/pull/4241#discussion_r989951601
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/impl/ConfigurationImpl.java:
##########
@@ -599,7 +746,46 @@ public <T> T convert(Class<T> type, Object value) {
BeanSupport.customise(beanUtils);
- beanUtils.populate(this, beanProperties);
+ logger.debug("populate:" + this + ", " + beanProperties);
Review Comment:
sorted, thanks!
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a diff in pull request #4241: ARTEMIS-4025 - trap failure to apply properties as errors and report …
Posted by GitBox <gi...@apache.org>.
gtully commented on code in PR #4241:
URL: https://github.com/apache/activemq-artemis/pull/4241#discussion_r993207211
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerStatus.java:
##########
@@ -0,0 +1,81 @@
+/**
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.activemq.artemis.core.server.impl;
+
+
+import java.util.HashMap;
+
+import org.apache.activemq.artemis.api.core.JsonUtil;
+import org.apache.activemq.artemis.api.core.SimpleString;
+import org.apache.activemq.artemis.json.JsonObject;
+import org.apache.activemq.artemis.json.JsonObjectBuilder;
+import org.apache.activemq.artemis.utils.JsonLoader;
+
+public class ServerStatus {
+
+ private static final ServerStatus instance = new ServerStatus();
+
+ public static synchronized ServerStatus getInstanceFor(ActiveMQServerImpl activeMQServer) {
+ if (instance.server == null) {
+ instance.server = activeMQServer;
+ instance.immutableStateValues.put("version", instance.server.getVersion().getFullVersion());
Review Comment:
I am going to leave the simple attributes, if we build up this some more, those names could be inferred.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a diff in pull request #4241: ARTEMIS-4025 - trap failure to apply properties as errors and report …
Posted by GitBox <gi...@apache.org>.
gtully commented on code in PR #4241:
URL: https://github.com/apache/activemq-artemis/pull/4241#discussion_r988908611
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/impl/ConfigurationImpl.java:
##########
@@ -538,7 +546,146 @@ public void parsePrefixedProperties(Properties properties, String prefix) throws
public void populateWithProperties(Map<String, Object> beanProperties) throws InvocationTargetException, IllegalAccessException {
CollectionAutoFillPropertiesUtil autoFillCollections = new CollectionAutoFillPropertiesUtil();
- BeanUtilsBean beanUtils = new BeanUtilsBean(new ConvertUtilsBean(), autoFillCollections);
+ BeanUtilsBean beanUtils = new BeanUtilsBean(new ConvertUtilsBean(), autoFillCollections) {
+ // override to treat missing properties as errors, not skip as the default impl does
+ @Override
+ public void setProperty(final Object bean, String name, final Object value) throws InvocationTargetException, IllegalAccessException {
+ {
+ if (logger.isTraceEnabled()) {
+ final StringBuilder sb = new StringBuilder(" setProperty(");
+ sb.append(bean);
+ sb.append(", ");
+ sb.append(name);
+ sb.append(", ");
+ if (value == null) {
+ sb.append("<NULL>");
+ } else if (value instanceof String) {
+ sb.append((String) value);
+ } else if (value instanceof String[]) {
+ final String[] values = (String[]) value;
+ sb.append('[');
+ for (int i = 0; i < values.length; i++) {
+ if (i > 0) {
+ sb.append(',');
+ }
+ sb.append(values[i]);
+ }
+ sb.append(']');
+ } else {
+ sb.append(value.toString());
+ }
+ sb.append(')');
+ logger.trace(sb.toString());
+ }
+
+ // Resolve any nested expression to get the actual target bean
+ Object target = bean;
+ final Resolver resolver = getPropertyUtils().getResolver();
+ while (resolver.hasNested(name)) {
+ try {
+ target = getPropertyUtils().getProperty(target, resolver.next(name));
+ if (target == null) {
+ throw new InvocationTargetException(null, "Resolved nested property for:" + name + ", on: " + bean + " was null");
+ }
+ name = resolver.remove(name);
+ } catch (final NoSuchMethodException e) {
+ throw new InvocationTargetException(e, "No getter for property:" + name + ", on: " + bean);
+ }
+ }
+ if (logger.isTraceEnabled()) {
+ logger.trace(" Target bean = " + target);
+ logger.trace(" Target name = " + name);
Review Comment:
I see there is logging on the brain, but fair... will tidy, thanks.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a diff in pull request #4241: ARTEMIS-4025 - trap failure to apply properties as errors and report …
Posted by GitBox <gi...@apache.org>.
gtully commented on code in PR #4241:
URL: https://github.com/apache/activemq-artemis/pull/4241#discussion_r988905681
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/impl/ConfigurationImpl.java:
##########
@@ -2852,13 +3038,14 @@ public Configuration setSuppressSessionNotifications(boolean suppressSessionNoti
}
@Override
- public String getStatus() {
- return status;
+ public synchronized String getStatus() {
Review Comment:
yep, not needed.. thanks
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4241: ARTEMIS-4025 - trap failure to apply properties as errors and report …
Posted by GitBox <gi...@apache.org>.
gemmellr commented on code in PR #4241:
URL: https://github.com/apache/activemq-artemis/pull/4241#discussion_r992158342
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/impl/ConfigurationImpl.java:
##########
@@ -523,22 +542,140 @@ public void parsePrefixedProperties(Properties properties, String prefix) throws
}
key = entry.getKey().toString().substring(prefix.length());
}
- String value = XMLUtil.replaceSystemPropsInString(entry.getValue().toString());
+ String value = entry.getValue().toString();
+
+ checksum.update(key.getBytes(StandardCharsets.UTF_8));
+ checksum.update('=');
+ checksum.update(value.getBytes(StandardCharsets.UTF_8));
+
+ value = XMLUtil.replaceSystemPropsInString(value);
value = PasswordMaskingUtil.resolveMask(isMaskPassword(), value, getPasswordCodec());
key = XMLUtil.replaceSystemPropsInString(key);
logger.debug("Property config, " + key + "=" + value);
beanProperties.put(key, value);
}
+ alder32Hash = checksum.getValue();
}
+ updateReadPropertiesStatus(name, alder32Hash);
if (!beanProperties.isEmpty()) {
- populateWithProperties(beanProperties);
+ populateWithProperties(name, beanProperties);
}
}
- public void populateWithProperties(Map<String, Object> beanProperties) throws InvocationTargetException, IllegalAccessException {
+ public void populateWithProperties(final String propsId, Map<String, Object> beanProperties) throws InvocationTargetException, IllegalAccessException {
CollectionAutoFillPropertiesUtil autoFillCollections = new CollectionAutoFillPropertiesUtil();
- BeanUtilsBean beanUtils = new BeanUtilsBean(new ConvertUtilsBean(), autoFillCollections);
+ BeanUtilsBean beanUtils = new BeanUtilsBean(new ConvertUtilsBean(), autoFillCollections) {
+ // override to treat missing properties as errors, not skip as the default impl does
+ @Override
+ public void setProperty(final Object bean, String name, final Object value) throws InvocationTargetException, IllegalAccessException {
+ {
+ logger.trace("setProperty on {}, name: {}, value: {}", bean.getClass(), name, value);
Review Comment:
As a 3-arg it should use isTraceEnabled() gate to avoid array overheads when not in use.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java:
##########
@@ -611,13 +614,24 @@ public void setProperties(String fileUrltoBrokerProperties) {
propertiesFileUrl = fileUrltoBrokerProperties;
}
+ @Override
+ public String getStatus() {
+ return serverStatus.asJson();
+ }
+
+ @Override
+ public void updateStatus(String component, String statusJson) {
+ serverStatus.update(component, statusJson);
+ }
+
private void internalStart() throws Exception {
if (state != SERVER_STATE.STOPPED) {
logger.debug("Server already started!");
return;
}
configuration.parseProperties(propertiesFileUrl);
+ updateStatus("configuration", configuration.getStatus());
Review Comment:
It could just be in the impl classes if its tied to the impl+tests, or a specific class for more general ones. Anywhere really, that helps tie the related bits together rather than expecting people will properly follow/associate literals later. This class has 2 related updates for "configuration" but thats not so obvious..whereas following a shared ref to the name would make it much more so.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/impl/ConfigurationImpl.java:
##########
@@ -599,7 +736,51 @@ public <T> T convert(Class<T> type, Object value) {
BeanSupport.customise(beanUtils);
- beanUtils.populate(this, beanProperties);
+ logger.trace("populate: bean: {} with {}", this, beanProperties);
+
+ HashMap<String, String> errors = new HashMap<>();
+ // Loop through the property name/value pairs to be set
+ for (final Map.Entry<String, ? extends Object> entry : beanProperties.entrySet()) {
+ // Identify the property name and value(s) to be assigned
+ final String name = entry.getKey();
+ try {
+ // Perform the assignment for this property
+ beanUtils.setProperty(this, name, entry.getValue());
+ } catch (InvocationTargetException invocationTargetException) {
+ logger.trace("failed to populate property with key: {}", name, invocationTargetException);
+ Throwable toLog = invocationTargetException;
+ if (invocationTargetException.getCause() != null) {
+ toLog = invocationTargetException.getCause();
+ }
+ trackError(errors, entry, toLog);
+
+ } catch (Exception oops) {
+ trackError(errors, entry, oops);
+ }
+ }
+ updateApplyStatus(propsId, errors);
+ }
+
+ private void trackError(HashMap<String, String> errors, Map.Entry<String,?> entry, Throwable oops) {
+ logger.debug("failed to populate property entry({}), reason: {}", entry.toString(), oops.getLocalizedMessage(), oops);
Review Comment:
Ditto
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a diff in pull request #4241: ARTEMIS-4025 - trap failure to apply properties as errors and report …
Posted by GitBox <gi...@apache.org>.
gtully commented on code in PR #4241:
URL: https://github.com/apache/activemq-artemis/pull/4241#discussion_r989955106
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java:
##########
@@ -611,13 +614,24 @@ public void setProperties(String fileUrltoBrokerProperties) {
propertiesFileUrl = fileUrltoBrokerProperties;
}
+ @Override
+ public String getStatus() {
+ return serverStatus.asJson();
+ }
+
+ @Override
+ public void updateStatus(String component, String statusJson) {
+ serverStatus.update(component, statusJson);
+ }
+
private void internalStart() throws Exception {
if (state != SERVER_STATE.STOPPED) {
logger.debug("Server already started!");
return;
}
configuration.parseProperties(propertiesFileUrl);
+ updateStatus("configuration", configuration.getStatus());
Review Comment:
maybe, not sure where to put them... they mostly should be a direct map from module/component name to status attribute, and within those objects - attribute are some subset of useful named values, using their names.
the "name" could be inferred really.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a diff in pull request #4241: ARTEMIS-4025 - trap failure to apply properties as errors and report …
Posted by GitBox <gi...@apache.org>.
gtully commented on code in PR #4241:
URL: https://github.com/apache/activemq-artemis/pull/4241#discussion_r993204103
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java:
##########
@@ -611,13 +614,24 @@ public void setProperties(String fileUrltoBrokerProperties) {
propertiesFileUrl = fileUrltoBrokerProperties;
}
+ @Override
+ public String getStatus() {
+ return serverStatus.asJson();
+ }
+
+ @Override
+ public void updateStatus(String component, String statusJson) {
+ serverStatus.update(component, statusJson);
+ }
+
private void internalStart() throws Exception {
if (state != SERVER_STATE.STOPPED) {
logger.debug("Server already started!");
return;
}
configuration.parseProperties(propertiesFileUrl);
+ updateStatus("configuration", configuration.getStatus());
Review Comment:
agree, thanks.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully merged pull request #4241: ARTEMIS-4025 - trap failure to apply properties as errors and report …
Posted by GitBox <gi...@apache.org>.
gtully merged PR #4241:
URL: https://github.com/apache/activemq-artemis/pull/4241
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on pull request #4241: ARTEMIS-4025 - trap failure to apply properties as errors and report …
Posted by GitBox <gi...@apache.org>.
gtully commented on PR #4241:
URL: https://github.com/apache/activemq-artemis/pull/4241#issuecomment-1275847541
thanks for all the feedback, appreciate your time.
--
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: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org