Blocked on #38

Issue

Now with our repo supporting multimodules, we have 3 pom files. We need to clean up these files and make sure we only have the dependencies, build plugins, and other aspects necessary for each module.

Also, we can reorganize some of these aspects so that we have less redundancy. For example, if both the workload and server share a dependency, we can declare this in the parent finapp module.

See: https://devflection.com/posts/2020-04-12-maven-part-3/

Next steps

1. Remove unnecessary dependencies, plugins, etc.
2. Reorganize pom files to reduce repetition

Issues

Currently, we throw IllegalArgumentException (an unchecked exception) for certain invalid inputs (i.e. negative amounts in moveAccountBalance). We catch these exceptions in FinAppService in order to provide useful feedback to users, but we could also be catching exceptions unrelated to the specific ones we're looking for (the ones we throw). We also expect these exceptions in our tests (namely, moveAccountBalance, which is being added soon) even though they are not in the signature.

A possible fix for this is creating a custom checked exception. This would mean that we could include it in our method signatures to then test and that we could specifically catch this exception.

Issue

with the new flag that switches between implementations and new command line instructions, we need to update the README.

• add documentation about how to switch the flag
• update commands about how to run the emulator
• specify separate terminal windows to run the emulator, grpc service, and to call the RPCs
• update commands about running the server

Issue

#38 introduces a workload generator, which indiscriminately makes 20 dollar transfers between accounts. Considering that the accounts start with a balance of 10,000, it will probably take a while for any invalid transfers to be made (taking 20 away from an account with a balance less than 20). The code will error out at this point. #50 will resolve the erroring out problem.

We need to decide if we want to send invalid queries to our server (which is something a potential user could do) and what percentage of queries we want to be valid/invalid. If queries are invalid, we also need to decide how to except and handle the errors.

see: https://github.com/GoogleCloudPlatform/cloud-spanner-samples/pull/38/files#r696173509, #38 (comment)

Next Steps

1. Decide if we want invalid queries to be generated
2. If we don't want invalid queries, come up with logic to guarantee that no invalid transfers are made
3. If we want invalid queries, guarantee that only some % are invalid
4. Differentiate between deterministic failures and non-deterministic failures

Issue

As of now, testing directly modifies the database that the application uses. Furthermore, we explicitly define the project, instance, and database IDs in the tests, rather than possibly getting this information from the environment.

We can also improve the tests to possibly use existing infrastructure to handle these problems and more. See: https://github.com/googleapis/java-spanner/blob/47f59655acaf0fcf5848039e6b4881b80cdceb5c/google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestEnv.java#L40

Next Steps

• Investigate IntegrationTestEnv to see if it or its ideas can be used for our tests
• Look into better ways to set project, instance, and database IDs, possibly using the environment
• Have tests possibly create and delete their own database

Issue

We represent monetary amounts as strings in our proto and then convert these strings to BigDecimal to process in Java. In the case that an invalid string is given as input in the constructor, a NumberFormatException is thrown. We handle this for createAccount by catching the exception in FinAppService. This problem will apply to future PRs that require an amount as input.

We should figure out a better way to handle this exception and provide useful output to users.

Next steps

1. Consider different solutions for handling this exception, including possibly adding it to signatures
2. Change current methods that use this BigDecimal constructor (createAccount, moveAccountBalance, and more)
3. Ensure that future methods that will run into this problem will handle the exception similarly

See #50 (comment) and #50 (comment).

Consider removing the task queue in the workload generator and instead generating new tasks on demand, or using a SynchronousQueue instead of a BlockingQueue in the executor as described in https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ThreadPoolExecutor.html.

Expected Behavior

Actual Behavior

Steps to Reproduce the Problem

Specifications

• Version:
• Platform:

Issue

In order to be able to use either implementation, SpannerDaoImpl or SpannerDaoJdbcImpl, the DatabaseModule class needs to bind both implementations (based on a flag?) to use one or the other from SpannerDaoInterface.

Next Steps

• edit DatabaseModule to bind one implementation based on a flag
• add a flag to ArgsModule and DatabaseModule

Issue

In SpannerDaoInterface's implementations and the test suite, we do not have any concrete debugging information printed or logged.

Steps to Reproduce the Problem

For SpannerDaoImpl, SpannerDaoJDBCImpl, and FinAppIT:

• Initialize the logger in the constructor
• Add useful log statements

Issue

Currently, the rpc GetRecentTransactionsForAccount does not have default timestamps. If only the begin_timestamp attribute is given, the default end_timestamp could be Timestamp.MAX_VALUE and vice versa.

Issue

#38 introduces the Commons CLI argument parser for the workload generator. This was chosen since it seems to be very widely used in Java applications. However, the server uses JCommander to parse arguments.

We should consider being consistent across the entire finapp and choose either JCommander or Commons CLI to use for both server and workload.

Issue

In moveAccountBalance of SpannerDaoImpl (and soon SpannerDaoJDBCImpl), we currently do not check for invalid inputs for amount, meaning we allow for a greater amount to be transferred from an account than what is in the account, making the account balance go negative. We should not allow account balances to go negative.

Next Steps

• Define output for invalid amount input
• Update SpannerDaoImpl
• Update SpannerDaoJDBCImpl (when merged)

There are several additional requirements that come out of PR #82 to improve the sample application.

• Implement testing against a Spanner instance instead of the emulator the PG app, since the emulator does not support PG interface.
• Adjust options for server type (eg Java, JDBC, PG) to be an enum, rather than adding even more flags.

Issue

currently, the RPC request/emthod names should be renamed (AddAccountForCustomer maybe not the best name, maybe say request in the name somewhere for the requests). also, the interface methods should return UUIDs.

Next Steps

• update rpc request/method names (and add responses now?)
• AddAccountForCustomer -> CreateCustomerRoleForAccount? (in FinAppService)
• createCustomer should return customerId
• createAccount should return accountId
• addAccountForCustomer shoulder return roleId
  this means changing the signatures across tests, interfaces, implementations, etc.

Issue

Currently, there are no unit tests for any implementation. Adding testing by mocking the emulator and/or grpc_cli will help ensure that the code runs as expected

Expected Behavior

when running the unit tests, switching the flag should result in using the correct implementation (either java client or JDBC)

Next Steps

• look into Mockito to mock grpc or emulator
• create unit tests to verify expected behavior
• this issue will become more detailed as the test files are pushed

Issue

#38 introduced a basic workload generator that uses the MoveAccountBalance and CreateAccount RPCs. We should include more of the methods from service.proto.

blocked on #38

Issue

#38 introduces a basic workload generator to make and send requests to the finance app gRPC server. It uses the same protobufs (since it is the protocol for communication between server and client). However, the PR copies the files into the workload submodule, since the compiler looks for the files in src/main/proto, which seems to be standard. Copying files is not ideal, since we'd like edits to RPCs to be shared between server and workload.

Next steps

1. Find a way to configure the compiler to specify where to get protobufs from
2. Move the protobufs to exist in a shared location, removing the copies

Issue

Without custom exceptions, the implementations of SpannerDaoInterface will not compile or build correctly as they throw different exceptions. Creating a custom exception class and throwing them will solve this issue.

Next Steps

• create custom exception class
• edit SpannerDaoInterface to throw custom exception
• edit SpannerDaoImpl and SpannerDaoJdbcImpl to throw and catch the custom exception
• replace any use of SpannerException in the finapp directory to use the custom exception

When FinApp Server is started in a Cloud Shell environment, the following Exception is thrown and it does not start.

According to the doc, when we use a Google Cloud cloud-based development environment such as Cloud Shell or Cloud Code, the applications running on the environment uses the credentials we provided when we logged in, and manages any authorizations required.

However, the grpc-java used by FinApp Server does not take this into account, so an error occurs when FinApp Server is started in a Cloud Shell environment.

As far as I've tried, using grpc-java v1.43.0+ instead of v1.38.0 solved this issue.

Since this application is used as the official Cloud Spanner sample, this fix should help users.

Expected Behavior

$ java -jar server/target/server-1.0-SNAPSHOT-jar-with-dependencies.jar  --spanner_project_id=<YOUR_PROJECT> --spanner_instance_id=<YOUR_INSTANCE> --spanner_database_id=finance-db
Dec 09, 2022 3:55:00 PM com.google.finapp.FinAppServer start
INFO: Server started, listening on 8080

Actual Behavior

$ java -jar server/target/server-1.0-SNAPSHOT-jar-with-dependencies.jar  --spanner_project_id=<YOUR_PROJECT> --spanner_instance_id=<YOUR_INSTANCE> --spanner_database_id=finance-db
Exception in thread "main" java.lang.NoSuchMethodError: 'io.grpc.alts.GoogleDefaultChannelCredentials$Builder io.grpc.alts.GoogleDefaultChannelCredentials.newBuilder()'
        at com.google.api.gax.grpc.InstantiatingGrpcChannelProvider.createSingleChannel(InstantiatingGrpcChannelProvider.java:326)
        at com.google.api.gax.grpc.ChannelPool.<init>(ChannelPool.java:105)
        at com.google.api.gax.grpc.ChannelPool.create(ChannelPool.java:83)
        at com.google.api.gax.grpc.InstantiatingGrpcChannelProvider.createChannel(InstantiatingGrpcChannelProvider.java:236)
        at com.google.api.gax.grpc.InstantiatingGrpcChannelProvider.getTransportChannel(InstantiatingGrpcChannelProvider.java:230)
        at com.google.api.gax.rpc.ClientContext.create(ClientContext.java:201)
        at com.google.cloud.spanner.v1.stub.GrpcSpannerStub.create(GrpcSpannerStub.java:231)
        at com.google.cloud.spanner.spi.v1.GapicSpannerRpc.<init>(GapicSpannerRpc.java:411)
        at com.google.cloud.spanner.spi.v1.GapicSpannerRpc.<init>(GapicSpannerRpc.java:302)
        at com.google.cloud.spanner.SpannerOptions$DefaultSpannerRpcFactory.create(SpannerOptions.java:469)
        at com.google.cloud.spanner.SpannerOptions$DefaultSpannerRpcFactory.create(SpannerOptions.java:464)
        at com.google.cloud.ServiceOptions.getRpc(ServiceOptions.java:560)
        at com.google.cloud.spanner.SpannerOptions.getSpannerRpcV1(SpannerOptions.java:1320)
        at com.google.cloud.spanner.SpannerImpl.<init>(SpannerImpl.java:132)
        at com.google.cloud.spanner.SpannerOptions$DefaultSpannerFactory.create(SpannerOptions.java:459)
        at com.google.cloud.spanner.SpannerOptions$DefaultSpannerFactory.create(SpannerOptions.java:454)
        at com.google.cloud.ServiceOptions.getService(ServiceOptions.java:540)
        at com.google.finapp.ServerMain.getSpannerDao(ServerMain.java:55)
        at com.google.finapp.ServerMain.main(ServerMain.java:33)

Steps to Reproduce the Problem

1. Open Cloud Shell environment.
2. Initialize finance-db
3. Run java -jar server/target/server-1.0-SNAPSHOT-jar-with-dependencies.jar --spanner_project_id=<YOUR_PROJECT> --spanner_instance_id=<YOUR_INSTANCE> --spanner_database_id=finance-db

Solution

• Use grpc-java v1.43.0 instead of v1.38.0
• grpc/grpc-java#8548

Specifications

• Version: latest as of 2022-12-09 #87
• Platform: Cloud Shell

Issue

As of now, our test suite requires that the user sets up and runs the emulator in a separate terminal. Ideally, we would have some command or some other method to automatically do that for the user so that they do not have to configure and run the emulator for themselves.

Possible methods (list in progress):

• AntRun Plugin

Next Steps

• Investigate methods
• Implement a strategy to lower user effort for running tests

Issue

Currently, the rpc needs 2 timestamps (begin and end) to query results.

Next Steps

Come up with a way to query results in a LIMIT BY way. Beware that Spanner can perform poorly executing certain SQL statements.

Issue

Because of the repo restructuring in #36, commands on the README might not work (such as when getting the schema.sdl, now must specify coming from server/src/main/java/com/google/finapp/schemda.sdl)

This also applies to the command to bring up the grpc service. It should now be java -jar target/server-1.0-SNAPSHOT-jar-with-dependencies.jar --spanner_project_id=test-project --spanner_instance_id=test-instance --spanner_database_id=test-database

Issue

https://github.com/GoogleCloudPlatform/cloud-spanner-samples/pull/38/files#r696150501:

Something to consider for the future: having a fixed QPS (i.e. "steady" load) in each worker by putting a delay here and warning if the call takes longer than the configured delay.

Issue

Currently, FinAppIT reuses a databaseClient.write method over and over again. The types and statuses are also unspecified, but this might not be able to be resolved until #47 is addressed.

Next Steps

• move method to a private method that can create accounts for the tests easily.
• make the account types and statuses not unspecified

Issue

2 methods are included in both implementations but are not yet implemented in FinAppService or service.proto

these methods include:

• addAccountForCustomer
• moveAccountBalance

Next Steps

in order to finish implementing these, we need to:

• add the rpc method to service.proto with its corresponding message request
• include the method in FinAppService by calling the method in SpannerDaoInterface with its appropriate parameters

Issue

some command line arguments need documentation to specify what to pass into ArgsModule, such as the implementation flag and what kind of arguments it can take.

[blocked on #50]

Issue

#50 made the workload generator multithreaded. However, this PR did not fine-tune the generator to make it as effective as possible. There are many different strategies to investigate for improving performance:

• Using non-blocking stubs (see: #50 (comment))
• Using separate channels per worker (see: #50 (comment))
• Changing the ThreadPoolExecutor or queue.
  • Creating a new handler (ThreadPoolExecutor documentation under rejected tasks) so that failed executor.submits block and retry (see: #50 (comment))
  • Removing the queue or adjusting the max queue size (see: #50 (comment))

Issue

Our RPC messages currently use the bytes type for ids (128 bit UUIDs). However, bytes can complicate code and make running the code more difficult. It would be easier to both read and run if we used the string type instead for ids.

Next steps

1. Change protobuf
2. Make changes in FinAppService, the SpannerDaoInterface and implementations, and wherever else is needed

Issue

As the application is defined now, there is no difference in behavior between the unspecified, active, and frozen account statuses, along with the unspecified, checking, and saving account types.

Next steps

1. Define the expected behavior for these different statuses/types for the different RPCs
2. If no difference in behavior is expected, consider the role and necessity these labels have in the app

Issue

Make SpannerDaoException class more restrictive by allowing exception to be constructed by SpannerException or SQLException by defining two constructors for these specific exceptions. This could help avoid wrapping unintended exceptions.