MappingWatcher should not close when mapper throws exception

[cormoran]

MappingWatcher stops watching when the mapper throws exception.
It's error-prone since the behavior is different from FileWatcher. FileWatcher retries watching when its mapper throws exception.

I'd like to suggest fixing MappingWatcher not to close on mapper exception.
It makes error handling in mapper function simpler in usual and the behavior matches to FileWatcher.

[jrhee17]

I agree the behavior is confusing and we probably want to align the behavior from the two implementations.

Just so I'm not missing anything, @minwoox do you know why MappingWatcher closing on an exception? I just glanced over the fluent API PR and am not sure if this is logic that has been copied over or not.

[minwoox]

Hi, @cormoran
The exception thrown from a FileWatcher is the one that is raised while watching a file. So it could be a network exception or any other exception that a user cannot control. That is why I've implemented it to retry.

centraldogma/client/java/src/main/java/com/linecorp/centraldogma/client/FileWatcher.java

Lines 62 to 63 in e93bc5a

	final CompletableFuture<Entry<T>> future = centralDogma.watchFile(
	projectName, repositoryName, lastKnownRevision, query, timeoutMillis, errorOnEntryNotFound);

On the other hand, the mapper is the one that a user implements. And I thought there was a contract that the mapper must not throw an exception.
However, there was no such contract so I think we can allow throwing an exception from the mapper. 😓

By the way, in which case are you throwing an exception from the mapper?

[cormoran]

By the way, in which case are you throwing an exception from the mapper?

@minwoox In our case, json parser throws exception if the json format is invalid like below.

centralDogma
    .forRepo("foo", "bar")
    .watcher(Query.ofJson("path"))
    .map(node -> {
            try {
                return new ObjectMapper().treeToValue(node, ConfigClass.class);
            } catch (JsonProcessingException e) {
                throw new RuntimeException(e); // here
            }
    })
    .start()

Some our mapper implementations re-throws exception by expecting infinite retry until the json format is fixed on central dogma like above.
Other implementations returns null from mapper function in case of failure and carefully handle null value in watcher listener to avoid hitting to MappingWatcher close.

We often use central dogma as dynamically reloadable setting repository (dynamically := reloadable without application restart).
For this purpose, below behaviors are nice:

• If the file format is invalid at application startup, startup should fail by awaitInitialValue timeout or exception.
• If invalid file change happened after application startup, the change should be ignored. Next valid change should be reflected to application.

To implement such behavior with minimum care, catching and ignoring mapper's exception by central dogma library (with warning log) is useful.

[minwoox]

Thanks for sharing your use case. 👍
Yeah, that definitely makes sense. 👍 Let us fix that soon.