Per-environment configuration for `ServerEnvironment` parts #1274

serhii-lekariev · 2020-05-19T11:37:03Z

`EnvSetting`

This PR introduces a new class for values that may differ between environments - EnvSetting<P>, which allows users to configure their value so that it's different between environments like so:

EnvSetting<StorageFactory> storage = new EnvSetting<>();
StorageFactory factory = InMemoryStorageFactory.newInstance();

// `Tests` is a type exposed by `base`. 
storage.use(factory, Tests.type());

Optional<StorageFactory> productionStorage = storage.value(Production.type());
assertThat(productionStorage).isEmpty();

Optional<StorageFactory> storageForTests = storage.value(Tests.type());
assertThat(storageForTests).isPresent();
assertThat(storageForTests.get()).isSameInstanceAs(factory);

`ServerEnvironment` changes

Previously, ServerEnvironment allowed to use different StorageFactory instances for different environments.

This functionality has been expanded to also allow configuration of:

Delivery: defaults to Delivery.local() if no value has been configured;
TracerFactory: does not default to any value, it's accessor still returns an Optional<TracerFactory>;
TransportFactory: defaults to an InMemoryTransportFactory for tests, still requires explicit configuration for all other environments.

Deprecation notes

The following ServerEnvironment methods have been deprecated:

configureStorageForTests: prefer use(storage, Tests.class);
configureStorage: prefer use(storage, Production.class); // or any other env type;
configureDelivery: prefer use(delivery, Production.class); // or any other env type.

Removed methods

ServerEnvironment#configureTransport(TransportFactory) was removed. ServerEnvironment#use(TransportFactory, Class<? extends EnvironmentType) can be used instead.

Fixes #615.

Update docs.

codecov · 2020-05-19T13:11:44Z

Codecov Report

Merging #1274 into master will decrease coverage by 0.05%.
The diff coverage is 81.51%.

@@             Coverage Diff              @@
##             master    #1274      +/-   ##
============================================
- Coverage     91.12%   91.07%   -0.06%     
- Complexity     4696     4710      +14     
============================================
  Files           605      606       +1     
  Lines         14920    14992      +72     
  Branches        852      851       -1     
============================================
+ Hits          13596    13654      +58     
- Misses         1057     1072      +15     
+ Partials        267      266       -1

armiol

@serhii-lekariev please see my comments.

Also, as you change is an API change, please make sure to bump the own library version.

armiol · 2020-05-19T13:45:12Z

server/src/main/java/io/spine/server/EnvironmentDependantValue.java

+ * @param <P>
+ *         the type of value
+ */
+public class EnvironmentDependantValue<P> {


This is way too complex for a name. Please think of some other options; if it's difficult to pick, leave a comment with any ideas that come to your mind. We'll then discuss.

Also, this class should be final.

ConfigurableValue is too generic, but sort of true?

It's a wrapper around two values, both of which may be absent, and those values have "production" and "testing" semantics, which I would call two different environments.

server/src/main/java/io/spine/server/EnvironmentDependantValue.java

armiol · 2020-05-19T13:48:43Z

server/src/main/java/io/spine/server/EnvironmentDependantValue.java

+public class EnvironmentDependantValue<P> {
+
+    private @Nullable P productionValue;
+    private final UnaryOperator<P> productionWrappingFunction;


Please shorten the name.

Why is this resolved?

server/src/main/java/io/spine/server/EnvironmentDependantValue.java

server/src/test/java/io/spine/server/EnvironmentDependantValueTest.java

version.gradle

armiol · 2020-05-19T14:00:49Z

server/src/test/java/io/spine/server/storage/system/SystemAwareStorageFactoryTest.java

    }

    @Test
    @DisplayName("wrap test storage")
    void wrapTestStorage() {
        ServerEnvironment serverEnv = ServerEnvironment.instance();
        StorageFactory testStorage = InMemoryStorageFactory.newInstance();
-        serverEnv.configureStorageForTests(testStorage);
+        serverEnv.configureStorage().tests(testStorage);


I would rather have something like

serverEnv.useStorageFactory(factory).inTests();

That's a lot closer to the way we speak.

…configured.

armiol

@serhii-lekariev please see my comments.

armiol · 2020-05-20T12:19:27Z

server/src/main/java/io/spine/server/EnvironmentDependantValue.java

+public class EnvironmentDependantValue<P> {
+
+    private @Nullable P productionValue;
+    private final UnaryOperator<P> productionWrappingFunction;


Why is this resolved?

server/src/main/java/io/spine/server/ServerEnvironment.java

armiol · 2020-05-20T12:26:07Z

server/src/main/java/io/spine/server/ServerEnvironment.java

@@ -313,11 +326,17 @@ public void close() throws Exception {
        if (tracerFactory != null) {
            tracerFactory.close();
        }
-        if (transportFactory != null) {
-            transportFactory.close();
+        if (transportFactories.production()


To elimitate the repeating if (... .production().isPresent()) { ...} please introduce the corresponding API to the EnvironmentDependantValue itself.

I can see at least four cases only in this class.

server/src/main/java/io/spine/server/EnvironmentDependantValue.java

…lose`.

…ances if possible. Add a hint to Javadocs. Partly convert the tests to a `Class<? extends EnvironmentType>` API.

armiol

@serhii-lekariev please see my comments.

armiol · 2020-06-09T16:03:08Z

server/src/main/java/io/spine/server/EnvSetting.java

+ * <p>For example:
+ * <pre>
+ *
+ * {@literal EnvSetting <StorageFactory>} storageFactory = new EnvSetting<>();


Let's kill that extra space character.

The extra symbol is still there.

Try to generate Javadocs themselves. Let's see if new EnvSetting<>(); portion renders as expected.

armiol · 2020-06-09T16:11:51Z

server/src/main/java/io/spine/server/EnvSetting.java

+    }
+
+    /**
+     * Represents an operation over a value that returns no result and may finish with an error.


This name is way too generic for this particular case. That's why I proposed to name it more specifically.

armiol · 2020-06-09T16:44:26Z

server/src/main/java/io/spine/server/EnvSetting.java

+     *
+     * <p>If it is not set, runs the specified supplier, configures and returns the supplied value.
+     */
+    V assignOrDefault(Supplier<V> defaultValue, EnvironmentType type) {


Now:

private final EnvSetting<MyFactory> setting = new EnvSetting<>(); //... return setting.assignOrDefault( // this method says it writes, but in fact it reads — and sometimes writes. () -> create(); type; );

Proposed change:

private final EnvSetting<MyFactory> setting = new EnvSetting<>(type, () -> create()); // default `fallback` value for `type`. private final EnvSetting<AnotherFactory> anotherSetting = new EnvSetting<>(); // no fallbacks. //... MyFactory value = setting.value(); // mandatory setting, e.g. `StorageFactory`; throws ISE if `empty()`. Optional<AnotherFactory> optionalValue = anotherSetting.optionalValue(); // optional setting, e.g. `Tracer`.

mention it in docs, clean up `ServerEnvironment`.

serhii-lekariev · 2020-06-12T15:49:04Z

@armiol, PTAL again. Build fails because this branch depends on the base 1.5.18. I can either decrease the version here, or wait until base is merged and published.

armiol · 2020-06-13T11:39:16Z

server/src/main/java/io/spine/server/EnvSetting.java

+ * <p>For example:
+ * <pre>
+ *
+ * {@literal EnvSetting <StorageFactory>} storageFactory = new EnvSetting<>();


The extra symbol is still there.

armiol · 2020-06-13T11:39:47Z

server/src/main/java/io/spine/server/EnvSetting.java

+ * <p>For example:
+ * <pre>
+ *
+ * {@literal EnvSetting <StorageFactory>} storageFactory = new EnvSetting<>();


Try to generate Javadocs themselves. Let's see if new EnvSetting<>(); portion renders as expected.

armiol · 2020-06-13T11:48:28Z

server/src/main/java/io/spine/server/EnvSetting.java

+ *         the type of value
+ */
+@Internal
+public final class EnvSetting<V> {


I don't understand why this class is public.

armiol · 2020-06-13T11:48:54Z

server/src/main/java/io/spine/server/delivery/Delivery.java

@@ -334,6 +338,10 @@ public static Delivery local() {
     * <p>The returned instance of {@code Delivery} is configured to use
     * {@linkplain UniformAcrossAllShards#singleShard() the single shard}.
     *
+     * <p>To construct a {@code Delivery} instance, a {@code StorageFactory} is needed.
+     * If it was not configured in the {@code ServerEnvironment}, uses a new {@code


armiol · 2020-06-13T11:49:30Z

server/src/main/java/io/spine/server/delivery/DeliveryBuilder.java

@@ -221,6 +225,9 @@ public DeliveryBuilder setInboxStorage(InboxStorage inboxStorage) {
     *
     * <p>If none set, the storage is initialized by the {@code StorageFactory} specific for
     * this {@code ServerEnvironment}.
+     *
+     * <p>If no {@code StorageFactory} is present in the {@code ServerEnvironment}, a new
+     * {@code InMemoryStorage} is used.


There is no such thing as InMemoryStorage. Please review your changeset for this copy-paste mistake.

armiol · 2020-06-13T11:50:29Z

server/src/test/java/io/spine/server/ServerEnvironmentTest.java

+                    .isInstanceOf(SystemAwareStorageFactory.class);
+            assertThat(((SystemAwareStorageFactory) serverEnvironment.storageFactory()).delegate())
+                    .isSameInstanceAs(storageFactory);
+


Please kill this line.

armiol · 2020-06-13T11:51:36Z

server/src/test/java/io/spine/server/ServerEnvironmentTest.java

+        void reset() {
+            serverEnvironment.reset();
+            Environment.instance()
+                       .register(new Local());


Not sure if this piece is migrated to the latest base API yet, but you may easily use a ctor instead of a new instance here.

armiol · 2020-06-13T11:52:58Z

server/src/test/java/io/spine/server/ServerEnvironmentTest.java

+            Environment.instance()
+                       .register(new Local());
+            Environment.instance()
+                       .setTo(new Local());


See how weird this is? You have several pieces of the same inconvenient code in this test. How much easier that would be if you could treat setTo as registration as well?

armiol · 2020-06-13T11:54:48Z

version.gradle.kts

@@ -25,6 +25,6 @@
 * as we want to manage the versions in a single source.
 */

-val spineBaseVersion: String by extra("1.5.12")
+val spineBaseVersion: String by extra("1.5.18")
 val spineTimeVersion: String by extra("1.5.12")


Let's migrate to the latest time once it's published.

Also, let's set the core-java version to be the same as base.

serhii-lekariev · 2020-06-13T14:48:45Z

@armiol, PTAL again.

armiol

@serhii-lekariev LGTM in general. Please see my comments. Also, make sure to use the latest base and time versions before this PR is merged.

armiol · 2020-06-14T11:49:30Z

server/src/main/java/io/spine/server/ServerEnvironment.java

@@ -47,6 +47,29 @@

 /**
 * The server conditions and configuration under which the application operates.
+ *
+ * <h1>Configuration</h1>
+ * <p>Some parts of the {@code ServerEnvironment} can be customized based on the {@code


Please have an empty line above.

armiol · 2020-06-14T11:49:46Z

server/src/main/java/io/spine/server/ServerEnvironment.java

+ *                      .use(testingStorageFactory, Tests.class)
+ *                      .use(memoizingTracerFactory, Production.class)
+ * </pre>
+ * A custom environment type may also be used:


Add <p> and one more empty line above.

armiol · 2020-06-14T11:50:06Z

server/src/main/java/io/spine/server/ServerEnvironment.java

+ *     // if `Staging` expects dependencies to its constructor.
+ *     ServerEnvironment.use(inMemoryStorageFactory, Staging.class);
+ * </pre>
+ * If {@code Staging} is {@link Environment#type() enabled}, the specified value is going to be


Start with <p> and add an empty line above.

armiol · 2020-06-14T11:50:38Z

server/src/main/java/io/spine/server/ServerEnvironment.java

+ * <p>Some parts of the {@code ServerEnvironment} can be customized based on the {@code
+ * EnvironmentType}. To do so, one of the overloads of the {@code use} method can be called.
+ * Two environment types exist out of the box: {@link Tests} and {@link Production}. For example:
+ * <pre>


Let's have an empty line above.

armiol · 2020-06-14T11:51:28Z

server/src/main/java/io/spine/server/EnvSetting.java

 *
 * <pre>
 *      // This `Supplier` is calculated only once.
 *     {@literal Supplier<StorageFactory>} fallbackStorage = InMemoryStorageFactory::newInstance;
 *
- *     {@literal EnvSetting<StorageFactory>} setting = {@literal new EnvSetting<>(Tests.class, fallbackStorage);}
+ *     {@literal EnvSetting<StorageFactory>} setting =


Can we fit this into a single literal and in a single line?

{@literal EnvSetting<StorageFactory> setting = new EnvSetting<>(Tests.class, fallbackStorage);}

yuri-sergiichuk · 2020-06-20T21:48:44Z

@serhii-lekariev so ServerEnvironment.configureTransport is removed completely. Maybe worth mention that? That's kind of a breaking change (a tiny one, but still).

serhii-lekariev · 2020-06-21T09:05:55Z

@serhii-lekariev so ServerEnvironment.configureTransport is removed completely. Maybe worth mention that? That's kind of a breaking change (a tiny one, but still).

I've updated the PR description.

Define an environment-dependant wrapper for any value.

377d1a2

serhii-lekariev self-assigned this May 19, 2020

serhii-lekariev added 3 commits May 19, 2020 14:52

Update config.

d22dbdf

Fix a wrong method call in tests.

70ac656

Expose per-environment configuration for the transport factory.

bb3296f

Update docs.

serhii-lekariev requested a review from armiol May 19, 2020 12:23

serhii-lekariev marked this pull request as ready for review May 19, 2020 12:23

Downgrade base.

3924eac

Fix build pattern usage.

4343dc3

armiol requested changes May 19, 2020

View reviewed changes

serhii-lekariev added 8 commits May 20, 2020 13:38

Allow serverEnv.useStorageFactory(factory).forProduction(); API.

ebb9e76

finalize a class, improve naming.

d9a399e

Fix Javadoc examples.

93903a5

Throw an IllegalStateException instead of NPE if the value was not …

801ac21

…configured.

Advance the version, due to the ServerEnvironment API change.

a4a528f

Make the tests more realistic.

c1a9636

Remind to bump the base version.

3e08a21

Update Javadoc examples.

0f92ca2

serhii-lekariev requested a review from armiol May 20, 2020 12:14

armiol requested changes May 20, 2020

View reviewed changes

serhii-lekariev added 8 commits May 20, 2020 15:36

Update Javadoc examples and exception messages.

498e86e

Rename and test a method.

7e09567

Add a utility method and tests it.

dea5536

Shorten a field name.

18adf08

Simplify working with the value wrapped in an EnvSetting.

c027f46

Write a simple test for ServerEnvironment.

d6ad178

Test whether the testing resources are closed on `ServerEnvironment#c…

510e297

…lose`.

Fix wording and formatting issues.

96d8403

serhii-lekariev requested a review from armiol May 20, 2020 13:47

serhii-lekariev added 2 commits June 9, 2020 17:12

Use EnvironmentType classes to avoid forcing callers to create inst…

4323921

…ances if possible. Add a hint to Javadocs. Partly convert the tests to a `Class<? extends EnvironmentType>` API.

Rewrite some of the test to use the new ServerEnvironment API.

284a221

armiol requested changes Jun 9, 2020

View reviewed changes

serhii-lekariev added 8 commits June 9, 2020 22:58

Force Tests environment for the delivery tests.

92aea44

Introduce the fallback functionality into the EnvSetting.

95a5252

Update config.

6a3cbf8

Configure delivery to create a storage factory if it needs it,

4d065bc

mention it in docs, clean up `ServerEnvironment`.

Use a correct ServerEnvironment API.

8d3a760

Use correct utils. Also license-report and pom.

1878a35

Rename a functional interface.

3a6e040

Fix formatting.

e69d5cb

serhii-lekariev requested a review from armiol June 12, 2020 15:49

armiol requested changes Jun 13, 2020

View reviewed changes

serhii-lekariev added 5 commits June 13, 2020 15:51

Fix Javadocs, user newer base API.

1e5d1f0

Fix Javadocs.

cd938eb

Fix old API usage, bump versions of core and time to match base.

fabd731

Revert unwanted changes, fix documentation errors.

9a004e2

Reword docs, fix formatting.

d6986b2

serhii-lekariev requested a review from armiol June 13, 2020 14:48

serhii-lekariev added 2 commits June 13, 2020 18:52

Extract a Javadoc paragraph on configuration.

266fccf

Fix Javadoc.

77b04ae

armiol approved these changes Jun 14, 2020

View reviewed changes

serhii-lekariev merged commit ed31f7e into master Jun 15, 2020

serhii-lekariev deleted the per-environment-storage-configuration-server-env branch June 16, 2020 14:09

armiol mentioned this pull request Jun 20, 2020

Initial ChatBot implementation SpineEventEngine/chat-bot#1

Merged

dmitrykuzmin mentioned this pull request Sep 8, 2020

Release 1.6.0 #1302

Merged

Per-environment configuration for ServerEnvironment parts #1274

Per-environment configuration for ServerEnvironment parts #1274

Conversation

serhii-lekariev commented May 19, 2020 • edited Loading

EnvSetting

ServerEnvironment changes

Deprecation notes

Removed methods

codecov bot commented May 19, 2020 • edited Loading

Codecov Report

armiol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armiol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armiol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhii-lekariev commented Jun 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhii-lekariev commented Jun 13, 2020

armiol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuri-sergiichuk commented Jun 20, 2020

serhii-lekariev commented Jun 21, 2020

Per-environment configuration for `ServerEnvironment` parts #1274

Per-environment configuration for `ServerEnvironment` parts #1274

serhii-lekariev commented May 19, 2020 •

edited

Loading

`EnvSetting`

`ServerEnvironment` changes

codecov bot commented May 19, 2020 •

edited

Loading