Skip to content

Entity Service and Config Service Configurable Timeouts #92

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

Merged
merged 5 commits into from
Jul 12, 2021
Merged

Entity Service and Config Service Configurable Timeouts #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
502e8c7
Entity service and config service configurable timeouts
suddendust Jul 11, 2021
f39ddd9
.synk update expiry date
suddendust Jul 12, 2021
22284e6
Fix typo
suddendust Jul 12, 2021
86bbfe5
Reuse optionallyGet in the implementation of getTimeoutOrFallback
suddendust Jul 12, 2021
d009bf9
chore: submodule ref update
suddendust Jul 12, 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
2 changes: 1 addition & 1 deletion .snyk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@ ignore:
SNYK-JAVA-IONETTY-1042268:
- '*':
reason: No replacement available
expires: 2021-06-30T00:00:00.000Z
expires: 2021-09-01T00:00:00.000Z
patch: {}

2 changes: 1 addition & 1 deletion hypertrace-core-graphql
Open in desktop
Submodule hypertrace-core-graphql updated 4 files
+1 −1 .snyk
+6 −2 ...race-core-graphql-attribute-store/src/main/java/org/hypertrace/core/graphql/attributes/AttributeClient.java
+18 −4 ...ace-core-graphql-service/src/main/java/org/hypertrace/core/graphql/service/DefaultGraphQlServiceConfig.java
+2 −0 hypertrace-core-graphql-spi/src/main/java/org/hypertrace/core/graphql/spi/config/GraphQlServiceConfig.java
5 changes: 5 additions & 0 deletions ...ce-config/src/main/java/org/hypertrace/graphql/config/HypertraceGraphQlServiceConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
package org.hypertrace.graphql.config;

import java.time.Duration;
import org.hypertrace.core.graphql.spi.config.GraphQlServiceConfig;

public interface HypertraceGraphQlServiceConfig extends GraphQlServiceConfig {
String getEntityServiceHost();

int getEntityServicePort();

Duration getEntityServiceTimeout();

String getConfigServiceHost();

int getConfigServicePort();

Duration getConfigServiceTimeout();
}
29 changes: 25 additions & 4 deletions ...hql-service/src/main/java/org/hypertrace/graphql/service/DefaultGraphQlServiceConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
@Value
class DefaultGraphQlServiceConfig implements HypertraceGraphQlServiceConfig {

private static final Long DEFAULT_CLIENT_TIMEOUT_SECONDS = 10L;

private static final String SERVICE_NAME_CONFIG = "service.name";
private static final String SERVICE_PORT_CONFIG = "service.port";

Expand All @@ -22,16 +24,19 @@ class DefaultGraphQlServiceConfig implements HypertraceGraphQlServiceConfig {

private static final String ATTRIBUTE_SERVICE_HOST_PROPERTY = "attribute.service.host";
private static final String ATTRIBUTE_SERVICE_PORT_PROPERTY = "attribute.service.port";
private static final String ATTRIBUTE_SERVICE_CLIENT_TIMEOUT = "attribute.service.timeout";

private static final String GATEWAY_SERVICE_HOST_PROPERTY = "gateway.service.host";
private static final String GATEWAY_SERVICE_PORT_PROPERTY = "gateway.service.port";
private static final String GATEWAY_SERVICE_CLIENT_TIMEOUT = "gateway.service.timeout";

private static final String ENTITY_SERVICE_HOST_PROPERTY = "entity.service.host";
private static final String ENTITY_SERVICE_PORT_PROPERTY = "entity.service.port";
private static final String ENTITY_SERVICE_CLIENT_TIMEOUT = "entity.service.timeout";

private static final String CONFIG_SERVICE_HOST_PROPERTY = "config.service.host";
private static final String CONFIG_SERVICE_PORT_PROPERTY = "config.service.port";
private static final String CONFIG_SERVICE_CLIENT_TIMEOUT = "config.service.timeout";

String serviceName;
int servicePort;
Expand All @@ -41,13 +46,16 @@ class DefaultGraphQlServiceConfig implements HypertraceGraphQlServiceConfig {
int maxIoThreads;
String attributeServiceHost;
int attributeServicePort;
Duration attributeServiceTimeout;
String gatewayServiceHost;
int gatewayServicePort;
Duration gatewayServiceTimeout;
String entityServiceHost;
int entityServicePort;
Duration entityServiceTimeout;
String configServiceHost;
int configServicePort;
Duration configServiceTimeout;

DefaultGraphQlServiceConfig(Config untypedConfig) {
this.serviceName = untypedConfig.getString(SERVICE_NAME_CONFIG);
Expand All @@ -65,11 +73,24 @@ class DefaultGraphQlServiceConfig implements HypertraceGraphQlServiceConfig {
this.entityServicePort = untypedConfig.getInt(ENTITY_SERVICE_PORT_PROPERTY);
this.configServiceHost = untypedConfig.getString(CONFIG_SERVICE_HOST_PROPERTY);
this.configServicePort = untypedConfig.getInt(CONFIG_SERVICE_PORT_PROPERTY);
// fallback timeout: 10s

this.gatewayServiceTimeout =
untypedConfig.hasPath(GATEWAY_SERVICE_CLIENT_TIMEOUT)
? untypedConfig.getDuration(GATEWAY_SERVICE_CLIENT_TIMEOUT)
: Duration.ofSeconds(10);
getSuppliedDurationOrFallback(
() -> untypedConfig.getDuration(GATEWAY_SERVICE_CLIENT_TIMEOUT));
this.attributeServiceTimeout =
getSuppliedDurationOrFallback(
() -> untypedConfig.getDuration(ATTRIBUTE_SERVICE_CLIENT_TIMEOUT));
this.configServiceTimeout =
getSuppliedDurationOrFallback(
() -> untypedConfig.getDuration(CONFIG_SERVICE_CLIENT_TIMEOUT));
this.entityServiceTimeout =
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this use anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

entityServiceTimeout is not being used anywhere. EntityTypeCachingClient uses a hardcoded value of 10s here. Should this value be replaced with this timeout?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was checking at the GQL level, if it is not used, should we add it or should we add it when it is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's a fair point. I just added it so that we have a consistent configuration (as all other services had a timeout). Should I remove it (we do have a default fallback value though so even if you don't give the config it'll work).

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 we can keep it for consistency as we are going with the different services that might have a different timeout requirement, and already have fields for (host, port) for this one.

getSuppliedDurationOrFallback(
() -> untypedConfig.getDuration(ENTITY_SERVICE_CLIENT_TIMEOUT));
}

private Duration getSuppliedDurationOrFallback(Supplier<Duration> durationSupplier) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same nit about using optionallyGet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed. Thanks :)

return optionallyGet(durationSupplier)
.orElse(Duration.ofSeconds(DEFAULT_CLIENT_TIMEOUT_SECONDS));
}

private <T> Optional<T> optionallyGet(Supplier<T> valueSupplier) {
Expand Down
22 changes: 15 additions & 7 deletions ...-schema/src/main/java/org/hypertrace/graphql/spaces/dao/ConfigServiceSpacesConfigDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
package org.hypertrace.graphql.spaces.dao;

import static java.util.concurrent.TimeUnit.SECONDS;

import io.grpc.CallCredentials;
import io.reactivex.rxjava3.core.Single;
import java.util.concurrent.TimeUnit;
import javax.inject.Inject;
import org.hypertrace.core.graphql.context.GraphQlRequestContext;
import org.hypertrace.core.graphql.utils.grpc.GraphQlGrpcContextBuilder;
Expand All @@ -19,11 +18,11 @@

class ConfigServiceSpacesConfigDao implements SpacesConfigDao {

private static final int DEFAULT_DEADLINE_SEC = 10;
private final SpacesConfigServiceFutureStub spaceConfigServiceStub;
private final GraphQlGrpcContextBuilder grpcContextBuilder;
private final SpaceConfigRulesRequestConverter requestConverter;
private final SpaceConfigRulesResponseConverter responseConverter;
private final HypertraceGraphQlServiceConfig serviceConfig;

@Inject
ConfigServiceSpacesConfigDao(
Expand All @@ -36,6 +35,7 @@ class ConfigServiceSpacesConfigDao implements SpacesConfigDao {
this.grpcContextBuilder = grpcContextBuilder;
this.requestConverter = requestConverter;
this.responseConverter = responseConverter;
this.serviceConfig = serviceConfig;

this.spaceConfigServiceStub =
SpacesConfigServiceGrpc.newFutureStub(
Expand All @@ -52,7 +52,9 @@ public Single<SpaceConfigRuleResultSet> getAllRules(GraphQlRequestContext reques
.callInContext(
() ->
this.spaceConfigServiceStub
.withDeadlineAfter(DEFAULT_DEADLINE_SEC, SECONDS)
.withDeadlineAfter(
serviceConfig.getConfigServiceTimeout().toMillis(),
TimeUnit.MILLISECONDS)
.getRules(this.requestConverter.convertGetRequest())))
.flatMap(this.responseConverter::convertGetResponse);
}
Expand All @@ -65,7 +67,9 @@ public Single<SpaceConfigRule> createRule(SpaceConfigRuleCreationRequest request
.callInContext(
() ->
this.spaceConfigServiceStub
.withDeadlineAfter(DEFAULT_DEADLINE_SEC, SECONDS)
.withDeadlineAfter(
serviceConfig.getConfigServiceTimeout().toMillis(),
TimeUnit.MILLISECONDS)
.createRule(this.requestConverter.convertCreationRequest(request))))
.flatMap(this.responseConverter::convertRule);
}
Expand All @@ -78,7 +82,9 @@ public Single<SpaceConfigRule> updateRule(SpaceConfigRuleUpdateRequest request)
.callInContext(
() ->
this.spaceConfigServiceStub
.withDeadlineAfter(DEFAULT_DEADLINE_SEC, SECONDS)
.withDeadlineAfter(
serviceConfig.getConfigServiceTimeout().toMillis(),
TimeUnit.MILLISECONDS)
.updateRule(this.requestConverter.convertUpdateRequest(request))))
.flatMap(this.responseConverter::convertRule);
}
Expand All @@ -91,7 +97,9 @@ public Single<Boolean> deleteRule(SpaceConfigRuleDeleteRequest request) {
.callInContext(
() ->
this.spaceConfigServiceStub
.withDeadlineAfter(DEFAULT_DEADLINE_SEC, SECONDS)
.withDeadlineAfter(
serviceConfig.getConfigServiceTimeout().toMillis(),
TimeUnit.MILLISECONDS)
.deleteRule(this.requestConverter.convertDeleteRequest(request))))
.flatMap(unusedResponse -> this.responseConverter.buildDeleteResponse());
}
Expand Down