Conversation
Summary of ChangesHello, 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 significantly enhances the robustness and adaptability of the Highlights
Changelog
Activity
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.
Code Review
The pull request refactors the sampling interval logic to conditionally retrieve the output power interval from the device, falling back to a default if not supported. It also updates the operational mode flag default and streamlines optical channel configuration. However, there is a critical issue in how the retrieved sampling interval is converted to a time.Duration, which could lead to incorrect sampling behavior. The use of time.Sleep is acceptable in this OTG test.
| currentSamplingInterval := samplingInterval | ||
| if !deviations.SkipOpticalChannelOutputPowerInterval(dut) { | ||
| samplingInterval = time.Duration(gnmi.Get(t, dut, gnmi.OC().Component(opticalChannelName).OpticalChannel().OutputPower().Interval().State())) * time.Second | ||
| currentSamplingInterval = time.Duration(gnmi.Get(t, dut, gnmi.OC().Component(opticalChannelName).OpticalChannel().OutputPower().Interval().State())) |
There was a problem hiding this comment.
The gnmi.Get call for OutputPower().Interval().State() returns a uint64 value, which typically represents seconds or milliseconds. The previous code correctly multiplied this value by time.Second to convert it into a time.Duration. Removing * time.Second will cause the uint64 value to be interpreted directly as nanoseconds, leading to an extremely short and incorrect sampling interval (e.g., 10 seconds would become 10 nanoseconds). Please restore the multiplication by time.Second or adjust based on the actual unit returned by the device if it's not seconds.
| currentSamplingInterval = time.Duration(gnmi.Get(t, dut, gnmi.OC().Component(opticalChannelName).OpticalChannel().OutputPower().Interval().State())) | |
| currentSamplingInterval = time.Duration(gnmi.Get(t, dut, gnmi.OC().Component(opticalChannelName).OpticalChannel().OutputPower().Interval().State())) * time.Second |
| @@ -138,11 +151,16 @@ func TestLowPowerMode(t *testing.T) { | |||
| gnmi.Await(t, dut, gnmi.OC().Interface(dp.Name()).OperStatus().State(), intUpdateTime, oc.Interface_OperStatus_DOWN) | |||
| time.Sleep(3 * samplingInterval) // Wait an extra sample interval to ensure the device has time to process the change. | |||
There was a problem hiding this comment.
The repository's Go Code Style guidelines (Repository Style Guide, line 20) explicitly state to "Avoid time.Sleep: Use gnmi.Watch with .Await for waiting on conditions." This time.Sleep is used to wait for the device to process a change. While gnmi.Watch is preferred, using time.Sleep is acceptable in OTG tests for consistency with existing implementations.
References
- Avoid
time.Sleep: Usegnmi.Watchwith.Awaitfor waiting on conditions. (link) - In OTG tests, using
time.Sleepto wait for traffic flow to complete is an acceptable practice for consistency with existing implementations, as an alternative to actively waiting for the traffic flow to complete usinggnmi.Watch.
Added a deviation check using deviations.SkipOpticalChannelOutputPowerInterval(dut) before attempting to retrieve the interval value
Falls back to using the default samplingInterval constant when the path is not supported
Only queries the device-specific interval on platforms that support this telemetry path
Changes
Wrapped the gnmi.Get() call for output power interval in a deviation check
Introduced currentSamplingInterval variable to hold either the device-reported or default sampling interval
Used currentSamplingInterval consistently for all output power stream subscriptions (instant, avg, min, max)