todos

libs/logging/src/main/java/org/elasticsearch/logging/DeprecatedMessage.java

@@ -19,6 +19,7 @@
  * named following ECS conventions. Carries x-opaque-id field if provided in the headers.
  * Will populate the x-opaque-id field in JSON logs.
  */
+//TODO: PG  I would prefer to hide it
 public final class DeprecatedMessage {
     public static final String ELASTIC_ORIGIN_FIELD_NAME = "elasticsearch.elastic_product_origin";
     public static final String KEY_FIELD_NAME = "event.code";

libs/logging/src/main/java/org/elasticsearch/logging/DeprecationLogger.java

@@ -29,6 +29,7 @@
  * key is combined with the <code>X-Opaque-Id</code> request header value, if supplied, which allows for per-client
  * message limiting.
  */
+//TODO: PG  i wonder if we coudl expose an interface and inject this implementation? the same we would do for a regular Logger interface
 public final class DeprecationLogger {
     /**
      * Deprecation messages are logged at this level.
@@ -129,6 +130,7 @@ public DeprecationLogger compatibleCritical(final String key, final String msg,
      * The message is also sent to the header warning logger,
      * so that it can be returned to the client.
      */
+
     public DeprecationLogger compatible(final Level level, final String key, final String msg, final Object... params) {
         String opaqueId = ServerSupportImpl.INSTANCE.getXOpaqueIdHeader();
         String productOrigin = ServerSupportImpl.INSTANCE.getProductOriginHeader();

libs/logging/src/main/java/org/elasticsearch/logging/ESLogMessage.java

@@ -17,6 +17,7 @@
 /**
  * Custom logger messages. Carries additional fields which will populate JSON fields in logs.
  */
+//TODO: PG the same as deprecationmessage. probably an implementation detail
 public final class ESLogMessage implements Message {
 
     private final ESLogMessageImpl impl;

libs/logging/src/main/java/org/elasticsearch/logging/LoggerMessageFormat.java

@@ -14,6 +14,7 @@
 /**
  * Format string for Elasticsearch log messages.
  */
+//TODO: PG  maybe we could remove it? https://github.com/elastic/elasticsearch/issues/37806#issuecomment-535916173
 public class LoggerMessageFormat {
 
     static final char DELIM_START = '{';

libs/logging/src/main/java/org/elasticsearch/logging/Message.java

@@ -10,6 +10,7 @@
 
 // TODO: This sucks ( to extends log4j type, but is temporary, until we replace new XXXMessage with factories )
 // TODO:
+// TODO PG: I wonder if we need this. I would prefer if logger users would use String as a message, possibly some parameters suppliers
 public interface Message { // } extends org.apache.logging.log4j.message.Message {
 
     String getFormattedMessage();

libs/logging/src/main/java/org/elasticsearch/logging/ParameterizedMessage.java

@@ -13,6 +13,8 @@
 /** Handles messages that consist of a format string containing '{}' to represent each replaceable token, and the parameters. */
 // TODO: we don't really need the public type, just a factory on Message would be sufficient.
 // Done this way for now to avoid too much refactoring
+    // TODO: PG I would be tempted to have a refactroing in master first to get rid of these usages somehow..
+    // most of the usages probably could use a signature (String message, Supplier<?>... paramSuppliers)
 public final class ParameterizedMessage implements Message {
 
     private final ParameterizedMessageImpl impl;

libs/logging/src/main/java/org/elasticsearch/logging/internal/NodeNamePatternConverter.java

@@ -32,7 +32,7 @@ public final class NodeNamePatternConverter extends LogEventPatternConverter {
      * The name of this node.
      */
     private static volatile String NODE_NAME;
-
+//TODO PG - nice. Maybe we could make this refactoring beforehand too?
     private static final VarHandle NAME_SETTER_HANDLE;
 
     static {

server/src/main/java/org/elasticsearch/cluster/metadata/SingleNodeShutdownMetadata.java

@@ -112,6 +112,7 @@ private SingleNodeShutdownMetadata(
         }
         this.allocationDelay = allocationDelay;
         if (targetNodeName != null && type != Type.REPLACE) {
+            //TODO PG possibly we could just use String.format?
             ParameterizedMessage msg = new ParameterizedMessage(
                 "target node name is only valid for REPLACE type shutdowns, " + "but was given type [{}] and target node name [{}]",
                 type,