-
Notifications
You must be signed in to change notification settings - Fork 135
feat: next release is v6.105.0-rc1 #4260
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.
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
base: protobuf-4.x-rc
Are you sure you want to change the base?
Conversation
#4252) * fix: Refine connecitivity metrics to capture RPCs with no response headers * test fix
Most transactions that exceed the mutation limit for an atomic transaction will fail with the error "The transaction contains too many mutations.". However, it is also possible that the transaction fails with the more generic error message "Transaction resource limits exceeded". This error did not trigger a retry of the statement using a PDML transaction. Fixes #4253
Summary of ChangesHello @diegomarquezp, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on internal improvements to the Google Cloud Spanner client library, specifically around how network latency metrics (GFE and AFE) are collected and reported, and how transaction limit exceptions are identified. The changes involve a significant refactoring of the metrics recording pipeline, moving header processing and metric recording to a more appropriate lifecycle stage within the gRPC interceptor. Additionally, it broadens the recognition of transaction mutation limit exceptions to include a more general resource limit message. These updates aim to enhance the accuracy and robustness of internal monitoring and error handling mechanisms, preparing the library for a new release candidate. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request prepares for the next release, v6.105.0-rc1. The changes primarily involve refactoring the built-in metrics recording logic, especially around server timing headers. This refactoring centralizes the logic, making it more robust and easier to maintain. The changes also include support for a new 'Transaction resource limits exceeded' error message.
My review has identified a couple of issues:
- Incorrect logging format in
HeaderInterceptor.javawhich would prevent log messages from being formatted correctly. - Use of wildcard imports in several files, which is against common Java style guidelines.
- A minor formatting issue in an XML configuration file.
I've provided suggestions to fix these issues. Overall, the refactoring is a good improvement.
| gfeLatency = serverTimingMetrics.get(GFE_TIMING_HEADER); | ||
| afeLatency = serverTimingMetrics.get(AFE_TIMING_HEADER); | ||
| } catch (NumberFormatException e) { | ||
| LOGGER.log(LEVEL, "Invalid server-timing object in header: {}", serverTiming); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logging format {} is for SLF4J, but java.util.logging.Logger is being used here. The correct format for java.util.logging.Logger is to use indexed placeholders like {0}. Using the wrong format will result in the placeholder being printed literally instead of being replaced by the variable's value.
| LOGGER.log(LEVEL, "Invalid server-timing object in header: {}", serverTiming); | |
| LOGGER.log(LEVEL, "Invalid server-timing object in header: {0}", serverTiming); |
| LOGGER.log( | ||
| LEVEL, "Unable to get built-in metric attributes {}", e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logging format {} is for SLF4J, but java.util.logging.Logger is being used here. The correct format for java.util.logging.Logger is to use indexed placeholders like {0}. Using the wrong format will result in the placeholder being printed literally instead of being replaced by the variable's value.
| LOGGER.log( | |
| LEVEL, "Unable to get built-in metric attributes {}", e.getMessage()); | |
| LOGGER.log( | |
| LEVEL, "Unable to get built-in metric attributes {0}", e.getMessage()); |
| <differenceType>7002</differenceType> | ||
| <className>com/google/cloud/spanner/CompositeTracer</className> | ||
| <method>void recordGFELatency(java.lang.Float)</method> | ||
| </difference><difference> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import io.grpc.Channel; | ||
| import io.grpc.ClientCall; | ||
| import io.grpc.ClientInterceptor; | ||
| import io.grpc.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve code clarity and avoid potential naming conflicts, it's a good practice to use explicit imports instead of wildcard imports. Please replace import io.grpc.*; with the specific classes used from the package.
| import io.grpc.*; | |
| import io.grpc.CallOptions; | |
| import io.grpc.Channel; | |
| import io.grpc.ClientCall; | |
| import io.grpc.ClientInterceptor; | |
| import io.grpc.Metadata; | |
| import io.grpc.MethodDescriptor; | |
| import io.grpc.Status; |
| import static com.google.cloud.spanner.DisableDefaultMtlsProvider.disableDefaultMtlsProvider; | ||
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertNotNull; | ||
| import static org.junit.Assert.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve code clarity and avoid potential naming conflicts, it's a good practice to use explicit imports instead of wildcard imports. Please replace import static org.junit.Assert.*; with the specific static methods used.
| import static org.junit.Assert.*; | |
| import static org.junit.Assert.assertEquals; | |
| import static org.junit.Assert.assertNotNull; | |
| import static org.junit.Assert.assertTrue; |
| import org.junit.AfterClass; | ||
| import org.junit.BeforeClass; | ||
| import org.junit.Test; | ||
| import org.junit.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve code clarity and avoid potential naming conflicts, it's a good practice to use explicit imports instead of wildcard imports. Please replace import org.junit.*; with the specific classes used from the package.
| import org.junit.*; | |
| import org.junit.After; | |
| import org.junit.AfterClass; | |
| import org.junit.BeforeClass; | |
| import org.junit.Test; |
| import org.junit.AfterClass; | ||
| import org.junit.BeforeClass; | ||
| import org.junit.Test; | ||
| import org.junit.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve code clarity and avoid potential naming conflicts, it's a good practice to use explicit imports instead of wildcard imports. Please replace import org.junit.*; with the specific classes used from the package.
| import org.junit.*; | |
| import org.junit.After; | |
| import org.junit.AfterClass; | |
| import org.junit.BeforeClass; | |
| import org.junit.Test; |
Includes sync with main, with commits 7b49412 and c735d42