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

Jump to bottom

fix: gax batching regression #863

Merged
merged 4 commits into from
Oct 25, 2021
Merged

fix: gax batching regression #863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
589c6a6
fix: gax batching regression
chanseokoh Oct 21, 2021
354a757
Fix bug and add tests
chanseokoh Oct 21, 2021
cbe8ab5
Update golden files to fix tests
chanseokoh Oct 24, 2021
77727a8
Revert some cleanups
chanseokoh Oct 25, 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
3 changes: 1 addition & 2 deletions src/main/java/com/google/api/generator/gapic/Generator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ public static CodeGeneratorResponse generateGapic(CodeGeneratorRequest request)
List<GapicClass> clazzes = Composer.composeServiceClasses(context);
GapicPackageInfo packageInfo = Composer.composePackageInfo(context);
String outputFilename = "temp-codegen.srcjar";
CodeGeneratorResponse response = Writer.write(context, clazzes, packageInfo, outputFilename);
return response;
return Writer.write(context, clazzes, packageInfo, outputFilename);
}
}
2 changes: 1 addition & 1 deletion src/main/java/com/google/api/generator/gapic/composer/Composer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public static List<GapicClass> generateMockClasses(GapicContext context, List<Se
services.forEach(
s -> {
if (context.transport() == Transport.REST) {
// REST transport tests donot not use mock services.
// REST transport tests do not use mock services.
} else if (context.transport() == Transport.GRPC) {
clazzes.add(MockServiceClassComposer.instance().generate(context, s));
clazzes.add(MockServiceImplClassComposer.instance().generate(context, s));
Expand Down
95 changes: 60 additions & 35 deletions ...google/api/generator/gapic/composer/common/AbstractTransportServiceStubClassComposer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,12 @@
import com.google.api.generator.engine.ast.VariableExpr;
import com.google.api.generator.gapic.composer.comment.StubCommentComposer;
import com.google.api.generator.gapic.composer.store.TypeStore;
import com.google.api.generator.gapic.composer.utils.ClassNames;
import com.google.api.generator.gapic.composer.utils.PackageChecker;
import com.google.api.generator.gapic.model.GapicClass;
import com.google.api.generator.gapic.model.GapicClass.Kind;
import com.google.api.generator.gapic.model.GapicContext;
import com.google.api.generator.gapic.model.GapicServiceConfig;
import com.google.api.generator.gapic.model.Message;
import com.google.api.generator.gapic.model.Method;
import com.google.api.generator.gapic.model.Service;
Expand Down Expand Up @@ -205,6 +207,7 @@ public GapicClass generate(GapicContext context, Service service) {
.setStatements(classStatements)
.setMethods(
createClassMethods(
context,
service,
typeStore,
classMemberVarExprs,
Expand Down Expand Up @@ -422,6 +425,7 @@ protected List<AnnotationNode> createClassAnnotations(Service service) {
}

protected List<MethodDefinition> createClassMethods(
GapicContext context,
Service service,
TypeStore typeStore,
Map<String, VariableExpr> classMemberVarExprs,
Expand All @@ -431,6 +435,7 @@ protected List<MethodDefinition> createClassMethods(
javaMethods.addAll(createStaticCreatorMethods(service, typeStore, "newBuilder"));
javaMethods.addAll(
createConstructorMethods(
context,
service,
typeStore,
classMemberVarExprs,
Expand Down Expand Up @@ -531,6 +536,7 @@ protected List<MethodDefinition> createStaticCreatorMethods(
}

protected List<MethodDefinition> createConstructorMethods(
GapicContext context,
Service service,
TypeStore typeStore,
Map<String, VariableExpr> classMemberVarExprs,
Expand Down Expand Up @@ -604,7 +610,9 @@ protected List<MethodDefinition> createConstructorMethods(
secondCtorExprs.add(
AssignmentExpr.builder()
.setVariableExpr(
classMemberVarExprs.get("callableFactory").toBuilder()
classMemberVarExprs
.get("callableFactory")
.toBuilder()
.setExprReferenceExpr(thisExpr)
.build())
.setValueExpr(callableFactoryVarExpr)
Expand All @@ -613,15 +621,16 @@ protected List<MethodDefinition> createConstructorMethods(
classMemberVarExprs.get(getTransportContext().transportOperationsStubNames().get(0));
// TODO: refactor this
if (generateOperationsStubLogic(service)) {
secondCtorExprs.addAll(createOperationsStubInitExpr(
service,
thisExpr,
operationsStubClassVarExpr,
clientContextVarExpr,
callableFactoryVarExpr));
secondCtorExprs.addAll(
createOperationsStubInitExpr(
service,
thisExpr,
operationsStubClassVarExpr,
clientContextVarExpr,
callableFactoryVarExpr));
}
secondCtorStatements.addAll(
secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()));
secondCtorExprs.stream().map(ExprStatement::withExpr).collect(Collectors.toList()));
secondCtorExprs.clear();
secondCtorStatements.add(EMPTY_LINE_STATEMENT);

Expand Down Expand Up @@ -660,7 +669,7 @@ protected List<MethodDefinition> createConstructorMethods(
protoMethodNameToDescriptorVarExprs.get(m.name())))
.collect(Collectors.toList()));
secondCtorStatements.addAll(
secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()));
secondCtorExprs.stream().map(ExprStatement::withExpr).collect(Collectors.toList()));
secondCtorExprs.clear();
secondCtorStatements.add(EMPTY_LINE_STATEMENT);

Expand All @@ -670,6 +679,8 @@ protected List<MethodDefinition> createConstructorMethods(
.map(
e ->
createCallableInitExpr(
context,
service,
e.getKey(),
e.getValue(),
callableFactoryVarExpr,
Expand All @@ -680,7 +691,7 @@ protected List<MethodDefinition> createConstructorMethods(
javaStyleMethodNameToTransportSettingsVarExprs))
.collect(Collectors.toList()));
secondCtorStatements.addAll(
secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()));
secondCtorExprs.stream().map(ExprStatement::withExpr).collect(Collectors.toList()));
secondCtorExprs.clear();
secondCtorStatements.add(EMPTY_LINE_STATEMENT);

Expand All @@ -705,7 +716,7 @@ protected List<MethodDefinition> createConstructorMethods(
.build())
.build());
secondCtorStatements.addAll(
secondCtorExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()));
secondCtorExprs.stream().map(ExprStatement::withExpr).collect(Collectors.toList()));
secondCtorExprs.clear();

// Second constructor method.
Expand All @@ -723,14 +734,14 @@ protected List<Expr> createOperationsStubInitExpr(
VariableExpr operationsStubClassVarExpr,
VariableExpr clientContextVarExpr,
VariableExpr callableFactoryVarExpr) {
TypeNode opeationsStubType = getTransportOperationsStubType(service);
TypeNode operationsStubType = getTransportOperationsStubType(service);
return Collections.singletonList(
AssignmentExpr.builder()
.setVariableExpr(
operationsStubClassVarExpr.toBuilder().setExprReferenceExpr(thisExpr).build())
.setValueExpr(
MethodInvocationExpr.builder()
.setStaticReferenceType(opeationsStubType)
.setStaticReferenceType(operationsStubType)
.setMethodName("create")
.setArguments(Arrays.asList(clientContextVarExpr, callableFactoryVarExpr))
.setReturnType(operationsStubClassVarExpr.type())
Expand All @@ -748,6 +759,8 @@ protected VariableExpr declareLongRunningClient() {
}

private Expr createCallableInitExpr(
GapicContext context,
Service service,
String callableVarName,
VariableExpr callableVarExpr,
VariableExpr callableFactoryVarExpr,
Expand All @@ -758,7 +771,7 @@ private Expr createCallableInitExpr(
Map<String, VariableExpr> javaStyleMethodNameToTransportSettingsVarExprs) {
boolean isOperation = callableVarName.endsWith(OPERATION_CALLABLE_NAME);
boolean isPaged = callableVarName.endsWith(PAGED_CALLABLE_NAME);
int sublength = 0;
int sublength;
if (isOperation) {
sublength = OPERATION_CALLABLE_NAME.length();
} else if (isPaged) {
Expand All @@ -767,11 +780,11 @@ private Expr createCallableInitExpr(
sublength = CALLABLE_NAME.length();
}
String javaStyleMethodName = callableVarName.substring(0, callableVarName.length() - sublength);
List<Expr> creatorMethodArgVarExprs = null;
List<Expr> creatorMethodArgVarExprs;
Expr transportSettingsVarExpr =
javaStyleMethodNameToTransportSettingsVarExprs.get(javaStyleMethodName);
if (transportSettingsVarExpr == null && isOperation) {
// Try again, in case the name dtection above was inaccurate.
// Try again, in case the name detection above was inaccurate.
isOperation = false;
sublength = CALLABLE_NAME.length();
javaStyleMethodName = callableVarName.substring(0, callableVarName.length() - sublength);
Expand Down Expand Up @@ -803,8 +816,9 @@ private Expr createCallableInitExpr(
clientContextVarExpr);
}

String methodName = JavaStyle.toUpperCamelCase(javaStyleMethodName);
Optional<String> callableCreatorMethodName =
getCallableCreatorMethodName(callableVarExpr.type());
getCallableCreatorMethodName(context, service, callableVarExpr.type(), methodName);

Expr initExpr;
if (callableCreatorMethodName.isPresent()) {
Expand All @@ -825,26 +839,37 @@ private Expr createCallableInitExpr(
.build();
}

protected Optional<String> getCallableCreatorMethodName(TypeNode callableVarExprType) {
final String typeName = callableVarExprType.reference().name();
String streamName = "Unary";

protected Optional<String> getCallableCreatorMethodName(
GapicContext context,
Service service,
TypeNode callableVarExprType,
String serviceMethodName) {
GapicServiceConfig serviceConfig = context.serviceConfig();
if (serviceConfig != null
&& serviceConfig.hasBatchingSetting(
service.protoPakkage(), service.name(), serviceMethodName)) {
return Optional.of("createBatchingCallable");
}
// Special handling for pagination methods.
if (callableVarExprType.reference().generics().size() == 2
&& callableVarExprType.reference().generics().get(1).name().endsWith("PagedResponse")) {
streamName = "Paged";
} else {
if (typeName.startsWith("Client")) {
streamName = "ClientStreaming";
} else if (typeName.startsWith("Server")) {
streamName = "ServerStreaming";
} else if (typeName.startsWith("Bidi")) {
streamName = "BidiStreaming";
} else if (typeName.startsWith("Operation")) {
streamName = "Operation";
}
return Optional.of("createPagedCallable");
}

String typeName = callableVarExprType.reference().name();
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: why is this style preferable? The older one (if-else) is technically shorter, so seems nicer.

Copy link
Contributor Author

@chanseokoh chanseokoh Oct 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, the new flow improves readability and reduces cognitive load. If you are familiar with the codebase, it may not matter, but the previous code requires you to read and understand the entire function to know what this is exactly doing. Technically the number of lines is shorter, but it requires you keep following all the contexts, as all the conditional flow must reach the last line. I needed to keep going up and down to make myself certain of the function's behavior. When you immediately return when a certain condition is met, you can stop thinking about the condition at all, reducing the cognitive load to ascertain what is going to happen.

Also, using String.format() makes you to think about it twice every time (sort of over-engineering for a very simple thing), while simply doing return <concrete value> is straight-forward and concisely signals what the function does and returns.

However, if you still prefer the current style, I can happily revert. Please let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's your thought?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I'm fine with either one, just wanted to make sure that the changes like this are done for a reason (otherwise, convergint one style to another without real purpose would be not a good way of spending time).

if (typeName.startsWith("Client")) {
return Optional.of("createClientStreamingCallable");
}
if (typeName.startsWith("Server")) {
return Optional.of("createServerStreamingCallable");
}
if (typeName.startsWith("Bidi")) {
return Optional.of("createBidiStreamingCallable");
}
if (typeName.startsWith("Operation")) {
return Optional.of("createOperationCallable");
}
return Optional.of(String.format("create%sCallable", streamName));
return Optional.of("createUnaryCallable");
}

private static List<MethodDefinition> createCallableGetterMethods(
Expand Down Expand Up @@ -960,7 +985,7 @@ private List<MethodDefinition> createStubOverrideMethods(
.setType(
TypeNode.withExceptionClazz(
IllegalStateException.class))
.setMessageExpr(String.format("Failed to close resource"))
.setMessageExpr("Failed to close resource")
.setCauseExpr(catchExceptionVarExpr)
.build())))
.build()))
Expand Down Expand Up @@ -1041,7 +1066,7 @@ private TypeStore createDynamicTypes(Service service, String stubPakkage) {
typeStore.putAll(
service.pakkage(),
service.methods().stream()
.filter(m -> m.isPaged())
.filter(Method::isPaged)
.map(m -> String.format(PAGED_RESPONSE_TYPE_NAME_PATTERN, m.name()))
.collect(Collectors.toList()),
true,
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/google/api/generator/util/Trie.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
* A common-prefix trie. T represents the type of each "char" in a word (which is a T-typed list).
*/
public class Trie<T> {
private class Node<U> {
private static class Node<U> {
final U chr;
// Maintain insertion order to enable deterministic test output.
Map<U, Node<U>> children = new LinkedHashMap<>();
Expand All @@ -39,7 +39,7 @@ private class Node<U> {
}
}

private Node<T> root;
private final Node<T> root;

public Trie() {
root = new Node<>();
Expand Down
18 changes: 15 additions & 3 deletions ...t/java/com/google/api/generator/gapic/composer/grpc/GrpcServiceStubClassComposerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,8 @@ public void generateGrpcServiceStubClass_deprecated() {
@Test
public void generateGrpcServiceStubClass_httpBindings() {
GapicContext context = GrpcTestProtoLoader.instance().parseShowcaseTesting();
Service testingProtoService = context.services().get(0);
GapicClass clazz =
GrpcServiceStubClassComposer.instance().generate(context, testingProtoService);
Service service = context.services().get(0);
GapicClass clazz = GrpcServiceStubClassComposer.instance().generate(context, service);

JavaWriterVisitor visitor = new JavaWriterVisitor();
clazz.classDefinition().accept(visitor);
Expand All @@ -79,4 +78,17 @@ public void generateGrpcServiceStubClass_httpBindingsWithSubMessageFields() {
Paths.get(Utils.getGoldenDir(this.getClass()), "GrpcPublisherStub.golden");
Assert.assertCodeEquals(goldenFilePath, visitor.write());
}

@Test
public void generateGrpcServiceStubClass_createBatchingCallable() {
GapicContext context = GrpcTestProtoLoader.instance().parseLogging();
Service service = context.services().get(0);
GapicClass clazz = GrpcServiceStubClassComposer.instance().generate(context, service);

JavaWriterVisitor visitor = new JavaWriterVisitor();
clazz.classDefinition().accept(visitor);
Utils.saveCodegenToFile(this.getClass(), "GrpcLoggingStub.golden", visitor.write());
Path goldenFilePath = Paths.get(Utils.getGoldenDir(this.getClass()), "GrpcLoggingStub.golden");
Assert.assertCodeEquals(goldenFilePath, visitor.write());
}
}
Loading