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

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 2 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);
}
}
8 changes: 3 additions & 5 deletions 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,11 +152,9 @@ 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.
} else if (context.transport() == Transport.GRPC) {
clazzes.add(MockServiceClassComposer.instance().generate(context, s));
clazzes.add(MockServiceImplClassComposer.instance().generate(context, s));
} else if (context.transport() == Transport.GRPC_REST) {
// REST transport tests do not use mock services.
} else if (context.transport() == Transport.GRPC
chanseokoh marked this conversation as resolved.
Show resolved Hide resolved
|| context.transport() == Transport.GRPC_REST) {
clazzes.add(MockServiceClassComposer.instance().generate(context, s));
clazzes.add(MockServiceImplClassComposer.instance().generate(context, s));
}
Expand Down
110 changes: 66 additions & 44 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 @@ -200,11 +202,11 @@ public GapicClass generate(GapicContext context, Service service) {
.setAnnotations(createClassAnnotations(service))
.setScope(ScopeNode.PUBLIC)
.setName(className)
.setExtendsType(
typeStore.get(getTransportContext().classNames().getServiceStubClassName(service)))
.setExtendsType(typeStore.get(ClassNames.getServiceStubClassName(service)))
chanseokoh marked this conversation as resolved.
Show resolved Hide resolved
.setStatements(classStatements)
.setMethods(
createClassMethods(
context,
service,
typeStore,
classMemberVarExprs,
Expand Down Expand Up @@ -422,6 +424,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 +434,7 @@ protected List<MethodDefinition> createClassMethods(
javaMethods.addAll(createStaticCreatorMethods(service, typeStore, "newBuilder"));
javaMethods.addAll(
createConstructorMethods(
context,
service,
typeStore,
classMemberVarExprs,
Expand Down Expand Up @@ -473,8 +477,7 @@ protected List<MethodDefinition> createStaticCreatorMethods(
argList ->
NewObjectExpr.builder().setType(creatorMethodReturnType).setArguments(argList).build();

TypeNode stubSettingsType =
typeStore.get(getTransportContext().classNames().getServiceStubSettingsClassName(service));
TypeNode stubSettingsType = typeStore.get(ClassNames.getServiceStubSettingsClassName(service));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this getting replaced? Is it solely because a static method is accessed via instance reference?

Note, since we already have ClassNames as part of transport context, all the static usages of ClassNames are potentially error-prone (a change in context one will not affect places, which are potentially the subject to change, because they are called statically), so basiclaly we need to convert it the other way around to reach consistency -> convert all Static ClassNames calls to dynamic instance-level ones (and making the method non-static).

The reason why those static methods are still static is historical.

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.

Is it solely because a static method is accessed via instance reference?

Yes. Code checking tools report that it is simply incorrect and error-prone to access a static method via an instance. (It makes people mistakenly assume that these calls are applying an instance context.)

potentially error-prone

I don't think the change make it error-prone. When it becomes the time you have to make the function non-static, the code will not compile anymore, so you'll have to update the code anyway.

However, I'll revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one's reverted.

VariableExpr settingsVarExpr =
VariableExpr.withVariable(
Variable.builder().setName("settings").setType(stubSettingsType).build());
Expand Down Expand Up @@ -531,13 +534,13 @@ protected List<MethodDefinition> createStaticCreatorMethods(
}

protected List<MethodDefinition> createConstructorMethods(
GapicContext context,
Service service,
TypeStore typeStore,
Map<String, VariableExpr> classMemberVarExprs,
Map<String, VariableExpr> callableClassMemberVarExprs,
Map<String, VariableExpr> protoMethodNameToDescriptorVarExprs) {
TypeNode stubSettingsType =
typeStore.get(getTransportContext().classNames().getServiceStubSettingsClassName(service));
TypeNode stubSettingsType = typeStore.get(ClassNames.getServiceStubSettingsClassName(service));
VariableExpr settingsVarExpr =
VariableExpr.withVariable(
Variable.builder().setName("settings").setType(stubSettingsType).build());
Expand Down Expand Up @@ -604,7 +607,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 +618,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 +666,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 +676,8 @@ protected List<MethodDefinition> createConstructorMethods(
.map(
e ->
createCallableInitExpr(
context,
service,
e.getKey(),
e.getValue(),
callableFactoryVarExpr,
Expand All @@ -680,7 +688,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 +713,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 +731,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 +756,8 @@ protected VariableExpr declareLongRunningClient() {
}

private Expr createCallableInitExpr(
GapicContext context,
Service service,
String callableVarName,
VariableExpr callableVarExpr,
VariableExpr callableFactoryVarExpr,
Expand All @@ -758,7 +768,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 +777,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 +813,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 +836,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 +982,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 @@ -1032,20 +1054,20 @@ private TypeStore createDynamicTypes(Service service, String stubPakkage) {
stubPakkage,
Arrays.asList(
getTransportContext().classNames().getTransportServiceStubClassName(service),
getTransportContext().classNames().getServiceStubSettingsClassName(service),
getTransportContext().classNames().getServiceStubClassName(service),
ClassNames.getServiceStubSettingsClassName(service),
ClassNames.getServiceStubClassName(service),
getTransportContext()
.classNames()
.getTransportServiceCallableFactoryClassName(service)));
// Pagination types.
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,
getTransportContext().classNames().getServiceClientClassName(service));
ClassNames.getServiceClientClassName(service));
return typeStore;
}

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