Support yaml partially

[Yang-33]

As far as I know and one of decaton users, inline comments are often necessary for CONFIG_IGNORE_KEYS, CONFIG_PARTITION_CONCURRENCY, and CONFIG_MAX_PENDING_RECORDS.

Using YAML allows us to add inline comment in central dogma decaton property file.
Decaton's internal parser is Jackson, which seems to partially support YAML. By leveraging this, YAML can be supported with relatively few changes.

Of course, it’s not complete. While YAML tags, anchors, and aliases cannot be used, we can include comments in the configuration file. Practically speaking, this seems to be sufficient...

If maintainers don't want to use YAML with parital support, there are some Jackson options that support JSON5 to support comment in json file.

• https://github.com/FasterXML/jackson-core/wiki/JsonReadFeatures#support-for-non-standard-json-content-comments

Resolve #173

[ocadaruma]

Hi, thank you for the patch and sorry for the late review.

Could you resolve the conflict, for the ease of review?

[Yang-33]

rebased!

[ocadaruma]

Left one early question, but overall strategy looks good.

Please go ahead and complete the implementation. Thanks for the patch!

[ocadaruma]

In contrast to current approach, new code sets the watch against entire file and iterates through all properties in the file.
Do you have any thoughts if it will cause some downside comparing to current approach?

[Yang-33]

I think there's almost no difference in practice!

The current decaton implementation uses Central Dogma’s MappingWatcher for each key/property, which attaches a child watcher to a root watcher.
When the root watcher's content (=JSON) changes, each child watcher is triggered independently.
All it does is extract the value for a specific key from the root JSON content and notify its listener only if the value has changed.

• https://github.com/line/centraldogma/blob/eb3c6d226f99e991fe54f6f67aa9d689af9a4994/client/java/src/main/java/com/linecorp/centraldogma/client/MappingWatcher.java#L66-L103

In this PR, it delegats that logic to DynamicProperty#checkingSet, which also compares the previous and new values and does not notify listeners unless there’s an actual change.

• decaton/processor/src/main/java/com/linecorp/decaton/processor/runtime/DynamicProperty.java

  Lines 71 to 116 in b795c1e

  	public T set(T value) {
  	setLock.lock();
  	try {
  	T currentValue = this.value;
  	if (currentValue == null && value == null || currentValue != null && currentValue.equals(value)) {
  	// No need to update.
  	return null;
  	}
  	validate(value);
  	this.value = value;
  	logger.debug("Property {} has been updated ({} => {})", name(), currentValue, value);
  	notifyListeners(currentValue, value);
  	return currentValue;
  	} finally {
  	setLock.unlock();
  	}
  	}
  	/**
  	* Update the value of this property, taking untyped object as an argument.
  	*
  	* This is a slightly different version of {@link #set}.
  	* Before calling {@link #set}, this method check if the passed value's runtime class is matching to the
  	* type configured as {@link PropertyDefinition#runtimeType()}.
  	* @param value new value to set to this property.
  	* @return old value which was set to this property. null if there was no update because the new value was
  	* equivalent to the current value.
  	*
  	* @throws IllegalArgumentException when invalid value passed.
  	*/
  	public T checkingSet(Object value) {
  	Class<?> runtimeType = definition().runtimeType();
  	if (value != null && runtimeType != null && !runtimeType.isInstance(value)) {
  	throw new IllegalArgumentException(String.format(
  	"type %s is not applicable for property %s of type %s",
  	value.getClass().getCanonicalName(), name(), runtimeType.getCanonicalName()));
  	}
  	return set(safeCast(value));
  	}

As for the listeners, the number of change notifications won’t differ. The only difference is which layer is responsible for delivering them (Central Dogma vs Decaton).

There might be some difference in terms of parallelism for notifying new value.
Decaton has about 15 properties, and with the current implementation, Central Dogma watchers can run in parallel (as long as the Central Dogma executor allows it).
This PR updates are currently handled sequentially, but considering that the keys and values are very small, the impact is likely negligible.

• decaton/processor/src/main/java/com/linecorp/decaton/processor/runtime/ProcessorProperties.java

  Lines 259 to 272 in d5dae11

  	CONFIG_IGNORE_KEYS,
  	CONFIG_PROCESSING_RATE,
  	CONFIG_PARTITION_CONCURRENCY,
  	CONFIG_MAX_PENDING_RECORDS,
  	CONFIG_COMMIT_INTERVAL_MS,
  	CONFIG_GROUP_REBALANCE_TIMEOUT_MS,
  	CONFIG_SHUTDOWN_TIMEOUT_MS,
  	CONFIG_LOGGING_MDC_ENABLED,
  	CONFIG_BIND_CLIENT_METRICS,
  	CONFIG_DEFERRED_COMPLETE_TIMEOUT_MS,
  	CONFIG_PROCESSOR_THREADS_TERMINATION_TIMEOUT_MS,
  	CONFIG_PER_KEY_QUOTA_PROCESSING_RATE,
  	CONFIG_RETRY_TASK_IN_LEGACY_FORMAT,
  	CONFIG_LEGACY_PARSE_FALLBACK_ENABLED));

[Yang-33]

oops, we might want to avoid using node.fields() in this change, and instead explicitly check properties based on ProcessorProperties#PROPERTY_DEFINITIONS.
There’s no benefit in processing keys that Decaton doesn’t care about. Let me modify this point, though it's not main part for your question...

[ocadaruma]

Thank you for the detailed explanation including the implementation of CD!

I see, then the strategy looks good.

[Yang-33]

Thank you. I've fixed failed tests while keeping the overall strategy. Please review when you have time 🙏

[ocadaruma]

Sorry for the late review.
Left only one minor comment. overall LGTM!

[ocadaruma]

This class doesn't need to be exposed to users, so let's move to internal package (refs: https://github.com/line/decaton/blob/master/VERSIONING.md#public-apis)

JsonFormat, YamlFormat as well

[Yang-33]

I plan to provide a simplified guide on the migration way for this. It will be reviewed separately from this PR and documented in the release notes.

[ocadaruma]

Oh is this breaking change? I thought no

[Yang-33]

Considering a case where there may be users with files that have a .yaml extension but contain JSON content, as mentioned in the PR description (although I don't think they exist...), it could be considered a breaking change.

While we can't prove their non-existence, we might assume they don't exist, and therefore, it might not need to be classified as a breaking change. Honestly, I'm quite conflicted about this...

[ocadaruma]

• Since YAML is a superset of JSON, even if the file has .yaml extension with JSON content, it should work even with new ver I guess
• In the first place, current version of CentralDogmaPropertySupplier instantiates the watcher with Query.ofJsonPath which will fail if the file doesn't ends with .json so no such user may exist

[Yang-33]

Query.ofJsonPath

I didn't notice it. As I checked now, it requires *.json as filename with validation finally.

• https://github.com/line/centraldogma/blob/194da137af1daa7bf53f23a9ddfa229580892265/common/src/main/java/com/linecorp/centraldogma/internal/Util.java#L44-L45
• https://github.com/line/centraldogma/blob/194da137af1daa7bf53f23a9ddfa229580892265/common/src/main/java/com/linecorp/centraldogma/internal/Util.java#L94-L98

Therefore there is no decaton user who uses JSON content with *.yaml filename.

Thank you so much!

[ocadaruma]

LGTM. Great work!