Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new method(overload) for CentralDogmaPropertySupplier#createPropertyFile/register #134

Merged
merged 22 commits into from
Dec 14, 2021
Merged

Add new method(overload) for CentralDogmaPropertySupplier#createPropertyFile/register #134

Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
320cd69
Add new method overload for createPropertyFile, register
lazmond3 Dec 2, 2021
3efe319
Change argument type StaticPropertySupplier to PropertySupplier
lazmond3 Dec 9, 2021
c17b3c7
Change return type of method: defaultProperties: List
lazmond3 Dec 9, 2021
df825c9
Add tests for CentralDogmaPropertySupplier#register
lazmond3 Dec 9, 2021
e8c311b
Add Javadoc for CentralDogmaPropertySupplier#register
lazmond3 Dec 10, 2021
6b49b94
Delete Unused CentralDogmaPropertySupplier#createPropertyFile
lazmond3 Dec 10, 2021
8d38efe
Reduce access to external systems in CentralDogmaPropertySupplier#reg…
lazmond3 Dec 10, 2021
03ef312
Move defaultProperties From CentralDogmaPropertySupplier To Processor…
lazmond3 Dec 10, 2021
6d9e69e
Use Property.ofStatic() instead of DynamicProperty
lazmond3 Dec 10, 2021
61cea29
Delete not meaningful verify method
lazmond3 Dec 10, 2021
382f706
Use primitive types int: CentralDogmaPropertySupplierTest
lazmond3 Dec 10, 2021
f2ee11d
Use any(String.class) for commit message of centraldogma
lazmond3 Dec 10, 2021
0f2db81
Use any(String.class) for commit message of centraldogma 2
lazmond3 Dec 13, 2021
2e70198
Reword javadoc for CentralDogmaPropertySupplier#register
lazmond3 Dec 13, 2021
f82bae3
Added a short javadoc for ProcessorProperties#defaultProperties
lazmond3 Dec 13, 2021
e852943
Fixed format in CentralDogmaPropertySupplierTest
lazmond3 Dec 13, 2021
ecf4284
Fix format in CentralDogmaPropertySupplierTest
lazmond3 Dec 14, 2021
e1952c4
Rename test private method: defaultPropertiesAsJsonNode
lazmond3 Dec 14, 2021
ed02a7c
Fix test: default properties list: not fragile: testRegisterWithCusto…
lazmond3 Dec 14, 2021
85f64d7
Merge remote-tracking branch 'origin/master' into feature/add-initial…
lazmond3 Dec 14, 2021
e6ae753
Provide only changed properties: testRegisterWithCustomizedSettings i…
lazmond3 Dec 14, 2021
bea0861
make the code short listPropertiesForVerifyingMock
lazmond3 Dec 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 52 additions & 2 deletions ...aldogma/src/main/java/com/linecorp/decaton/centraldogma/CentralDogmaPropertySupplier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,14 @@

package com.linecorp.decaton.centraldogma;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.stream.Collectors;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -41,6 +44,7 @@
import com.linecorp.decaton.processor.runtime.Property;
import com.linecorp.decaton.processor.runtime.PropertyDefinition;
import com.linecorp.decaton.processor.runtime.PropertySupplier;
import com.linecorp.decaton.processor.runtime.StaticPropertySupplier;

/**
* A {@link PropertySupplier} implementation with Central Dogma backend.
Expand Down Expand Up @@ -119,7 +123,7 @@ public <T> Optional<Property<T>> getProperty(PropertyDefinition<T> definition) {
try {
JsonNode node = child.initialValueFuture().join().value(); //doesn't fail since it's a child watcher
setValue(prop, node);
} catch (RuntimeException e) {
} catch (RuntimeException e) {
logger.warn("Failed to set initial value from CentralDogma for {}", definition.name(), e);
}

Expand All @@ -145,19 +149,43 @@ public static CentralDogmaPropertySupplier register(CentralDogma centralDogma, S
return new CentralDogmaPropertySupplier(centralDogma, project, repository, filename);
}

public static CentralDogmaPropertySupplier register(CentralDogma centralDogma, String project,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As its a public interface, please give it a javadoc like another overload of register.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a javadoc for the new added register method: e8c311b

String repository, String filename,
StaticPropertySupplier supplier) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in most cases people would use StaticPropertySupplier anyway, but we don't necessarily restrict this to be it, and just PropertySupplier (interface) is fine and more flexible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3efe319
I agree with you, and changed the argument type of supplier from StaticPropertySupplier to PropertySupplier.

List<Property<?>> properties = defaultPropertiesAsList().stream().map(defaultProperty -> {
if (supplier.getProperty(defaultProperty.definition()).isPresent()) {
return supplier.getProperty(defaultProperty.definition()).get();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a call of getProperty may involves I/O with external systems (like centraldogma), it is better to keep count of calling it as minimum as possible.
so let's do it like:

prop = supplier.getProperty(...);
if (prop.ifPresent()) {
    return prop.get();
}
...

Copy link
Contributor Author

@lazmond3 lazmond3 Dec 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the code to use a temporary variable for keeping the value in external systems. 8d38efe

} else {
return defaultProperty;
}
}).collect(Collectors.toList());

createPropertyFile(centralDogma, project, repository, filename, properties);
return new CentralDogmaPropertySupplier(centralDogma, project, repository, filename);
}

private static void createPropertyFile(CentralDogma centralDogma, String project,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this overload no longer meaningful. Let's just use the below signature and pass default properties directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted this original method createPropertyFile (no longer used), since a new method which has more arguments was added. 6b49b94

String repository, String fileName) {
createPropertyFile(centralDogma,
project, repository, fileName, defaultPropertiesAsList());
}

private static void createPropertyFile(CentralDogma centralDogma, String project,
String repository, String fileName,
List<Property<?>> properties) {
Revision baseRevision = normalizeRevision(centralDogma, project, repository, Revision.HEAD);
boolean fileExists = fileExists(centralDogma, project, repository, fileName, baseRevision);
long startedTime = System.currentTimeMillis();
long remainingTime = remainingTime(PROPERTY_CREATION_TIMEOUT_MILLIS, startedTime);

JsonNode jsonNodeProperties = convertPropertyListToJsonNode(properties);

while (!fileExists && remainingTime > 0) {
try {
centralDogma.push(project, repository, baseRevision,
String.format("[CentralDogmaPropertySupplier] Property file created: %s",
fileName),
Change.ofJsonUpsert(fileName, defaultProperties()))
Change.ofJsonUpsert(fileName, jsonNodeProperties))
.get(remainingTime, TimeUnit.MILLISECONDS);
logger.info("New property file registered on Central Dogma: {}/{}/{}",
project, repository, fileName);
Expand Down Expand Up @@ -224,6 +252,19 @@ private static long remainingTime(long totalTime, long startedTime) {
return totalTime - (System.currentTimeMillis() - startedTime);
}

private static JsonNode convertPropertyListToJsonNode(List<Property<?>> properties) {
final ObjectNode propertiesObjectNode = objectMapper.createObjectNode();
properties.forEach(
property -> {
propertiesObjectNode.set(
property.definition().name(),
objectMapper.valueToTree(property.value())
);
}
);
return propertiesObjectNode;
}

// visible for testing
static JsonNode defaultProperties() {
final ObjectNode properties = objectMapper.createObjectNode();
Expand All @@ -234,4 +275,13 @@ static JsonNode defaultProperties() {

return properties;
}

// visible for testing
static List<Property<?>> defaultPropertiesAsList() {
List<Property<?>> properties = new ArrayList<>();
ProcessorProperties.PROPERTY_DEFINITIONS
.forEach(definition -> properties.add(new DynamicProperty(definition)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no point to use DynamicProperty here. It's for modifiable property instance, so let's use Property.ofStatic() instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6d9e69e


return properties;
}
}