Adds Event and EventBuilder where changes are made in batch/bulk and …

avaje / avaje-config

Application configuration / properties loading for JVM applications

https://avaje.io/config

Apache License 2.0

55 stars 8 forks source link

Adds Event and EventBuilder where changes are made in batch/bulk and … #57

Closed rbygrave closed 1 year ago

rbygrave commented 1 year ago

…onChange() listener gets all the changes after they are all applied

Leaves all the existing setProperty(), clearProperty(), onChange() in place and unchanged
With the current approach could remove the 'snapshot copy' as EventBuilder has no get() read methods

SentryMan commented 1 year ago

Leaves all the existing setProperty(), clearProperty(), onChange() in place and unchanged

Sweet, I kinda liked those

rbygrave commented 1 year ago

Next step: Modify the FileWatcher to use the new bulk method - load everything into Map<String,String> or Properties (without any eval). eventBuilder() .put() ... alll of the key/values ... publish()

Next next step: Modify setProperty() to be eventBuilder().put(key, value).publish(); Modify clearProperty() to be eventBuilder().remove(key).publish(); Modify the existing per property listeners to work off Event

Next next next step: Review and deprecate what we really don't want folks to use longer term

rbygrave commented 1 year ago

Ok, I think that's it for now.

Followup:

Look to deprecate the onChangeInt(), onChangeLong(), onChangeBool(), onChange[String]() methods. I think it would be better to just have the one onChange() method down the track.

agentgt commented 1 year ago

@rbygrave great stuff! I added some comments on the commits to try be of some use.

The big one is I would make the listeners list a concurrent collection e.g. CopyOnWriteArrayList will probably fine.

The second one is I'm not sure about CoreEntry having a toString including the value its holding as in passwords could be in there. I'm not sure how likely or if it is possible toString CoreEntry externally right now but I think it is best to be safe on this.

A last one but I think it was @SentryMan change is to requireNonNull on the Configuration.forPath argument as might get null.somepath.

https://github.com/avaje/avaje-config/blob/136fe39315fc120ebdc7b4b9cd76dc2f85725615/avaje-config/src/main/java/io/avaje/config/CoreConfiguration.java#L151

agentgt commented 1 year ago

It also might be useful to have the listener executed in a pluggable executor this could somewhat guarantee some ordering kind of.

That is you could lock it so only one thread does events with that option.

rbygrave commented 1 year ago

Done the changes for CopyOnWriteArrayList and toString. Thanks for those points.

might be useful to have the listener executed in a pluggable executor this could somewhat guarantee some ordering kind of

Hmmm, I'll ponder this. We could make the invoke listeners run serially by moving the lock that is currently on CoreEventMap to CoreConfiguration.publishEvents().

rbygrave commented 1 year ago

useful to have the listener executed in a pluggable executor

This isn't addressed yet. I'll look to merge this in and do a separate PR for this as I think this leads to a couple of renames and I think it will be good with a clean diff.

rbygrave commented 1 year ago

useful to have the listener executed in a pluggable executor

This is in https://github.com/avaje/avaje-config/pull/60 ... as ModificationEventRunner. Plus this PR includes a rename of Event to ModificationEvent as I felt Event was a touch generic when this part is really about 'changes / modifications' so ModificationEvent seems a bit more explicit in a good way.