[Merged by Bors] - Use AwaitLayer instead of subscription in beacon #4013
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
noamnelke
approved these changes
Jan 29, 2023
This was referenced Jan 29, 2023
7 tasks
|
bors try |
tryBuild failed: |
|
bors merge |
bors bot
pushed a commit
that referenced
this pull request
Feb 1, 2023
## Motivation Part of #4010. Refactor code to not subscribe to layer changes but rather await new layers. ## Changes Use `AwaitLayer` in `Beacon` instead of a layer timer and make sure that the correct layer is processed by `ProtocolDriver`: - `AwaitLayer` might return late (e.g. if the node was hibernating while it was waiting for the next epoch) in this case `ProtocolDriver` will continue with the current epoch rather than the one it was awaiting. - `GetCurrentLayer` might return a layer lower than the one awaited (if a time sync happens right between awaiting and getting the current layer). In this case wait a little longer. - `listenEpochs` now fires once per epoch instead of once per layer and then checking if the layer is the first in an epoch. This should make the code a bit more efficient. ## Test Plan n/a ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
|
Build failed: |
|
bors merge |
bors bot
pushed a commit
that referenced
this pull request
Feb 1, 2023
## Motivation Part of #4010. Refactor code to not subscribe to layer changes but rather await new layers. ## Changes Use `AwaitLayer` in `Beacon` instead of a layer timer and make sure that the correct layer is processed by `ProtocolDriver`: - `AwaitLayer` might return late (e.g. if the node was hibernating while it was waiting for the next epoch) in this case `ProtocolDriver` will continue with the current epoch rather than the one it was awaiting. - `GetCurrentLayer` might return a layer lower than the one awaited (if a time sync happens right between awaiting and getting the current layer). In this case wait a little longer. - `listenEpochs` now fires once per epoch instead of once per layer and then checking if the layer is the first in an epoch. This should make the code a bit more efficient. ## Test Plan n/a ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
|
Pull request successfully merged into develop. Build succeeded: |
bors
bot
changed the title
bors bot
pushed a commit
that referenced
this pull request
Feb 4, 2023
## Motivation Closes #4010, this is a follow up to #4012, #4013, #4014 and removes the now obsolete pub/sub mechanism of the internal clock. ## Changes The core of the change is the `timesync` package, the rest of the PR is adapting the existing code to the new interface of the clock. `AwaitLayer` and `Subscribe` have the same purpose, but behave differently. This PR removes the duplicated code and simplifies the clock: - `Subscribe`/`Unsubscribe` and all logic associated with these two methods has been removed - `Ticker` and `TimeClock` have been merged into `NodeClock` - Methods `GetGenesisTime` and `GetCurrentLayer` have been renamed to `GenesisTime` and `CurrentLayer`. - The clock itself is now ticking once at the beginning of a layer and adjusts the waiting period until the next layer based on when the tick was issued. ## Test Plan Tests for `NodeClock` have been written from scratch, with emphasis on safe concurrent use and stability against time shifts that might be caused by NTP or the node hibernating. For this I added a dependency on http://github.com/benbjohnson/clock (was already an indirect dependency). It allows to mock time more easily in tests than it would be possible with `gomock`. ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot
pushed a commit
that referenced
this pull request
Feb 4, 2023
## Motivation Closes #4010, this is a follow up to #4012, #4013, #4014 and removes the now obsolete pub/sub mechanism of the internal clock. ## Changes The core of the change is the `timesync` package, the rest of the PR is adapting the existing code to the new interface of the clock. `AwaitLayer` and `Subscribe` have the same purpose, but behave differently. This PR removes the duplicated code and simplifies the clock: - `Subscribe`/`Unsubscribe` and all logic associated with these two methods has been removed - `Ticker` and `TimeClock` have been merged into `NodeClock` - Methods `GetGenesisTime` and `GetCurrentLayer` have been renamed to `GenesisTime` and `CurrentLayer`. - The clock itself is now ticking once at the beginning of a layer and adjusts the waiting period until the next layer based on when the tick was issued. ## Test Plan Tests for `NodeClock` have been written from scratch, with emphasis on safe concurrent use and stability against time shifts that might be caused by NTP or the node hibernating. For this I added a dependency on http://github.com/benbjohnson/clock (was already an indirect dependency). It allows to mock time more easily in tests than it would be possible with `gomock`. ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot
pushed a commit
that referenced
this pull request
Feb 4, 2023
## Motivation Closes #4010, this is a follow up to #4012, #4013, #4014 and removes the now obsolete pub/sub mechanism of the internal clock. ## Changes The core of the change is the `timesync` package, the rest of the PR is adapting the existing code to the new interface of the clock. `AwaitLayer` and `Subscribe` have the same purpose, but behave differently. This PR removes the duplicated code and simplifies the clock: - `Subscribe`/`Unsubscribe` and all logic associated with these two methods has been removed - `Ticker` and `TimeClock` have been merged into `NodeClock` - Methods `GetGenesisTime` and `GetCurrentLayer` have been renamed to `GenesisTime` and `CurrentLayer`. - The clock itself is now ticking once at the beginning of a layer and adjusts the waiting period until the next layer based on when the tick was issued. ## Test Plan Tests for `NodeClock` have been written from scratch, with emphasis on safe concurrent use and stability against time shifts that might be caused by NTP or the node hibernating. For this I added a dependency on http://github.com/benbjohnson/clock (was already an indirect dependency). It allows to mock time more easily in tests than it would be possible with `gomock`. ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot
pushed a commit
that referenced
this pull request
Feb 4, 2023
## Motivation Closes #4010, this is a follow up to #4012, #4013, #4014 and removes the now obsolete pub/sub mechanism of the internal clock. ## Changes The core of the change is the `timesync` package, the rest of the PR is adapting the existing code to the new interface of the clock. `AwaitLayer` and `Subscribe` have the same purpose, but behave differently. This PR removes the duplicated code and simplifies the clock: - `Subscribe`/`Unsubscribe` and all logic associated with these two methods has been removed - `Ticker` and `TimeClock` have been merged into `NodeClock` - Methods `GetGenesisTime` and `GetCurrentLayer` have been renamed to `GenesisTime` and `CurrentLayer`. - The clock itself is now ticking once at the beginning of a layer and adjusts the waiting period until the next layer based on when the tick was issued. ## Test Plan Tests for `NodeClock` have been written from scratch, with emphasis on safe concurrent use and stability against time shifts that might be caused by NTP or the node hibernating. For this I added a dependency on http://github.com/benbjohnson/clock (was already an indirect dependency). It allows to mock time more easily in tests than it would be possible with `gomock`. ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot
pushed a commit
that referenced
this pull request
Feb 5, 2023
## Motivation Closes #4010, this is a follow up to #4012, #4013, #4014 and removes the now obsolete pub/sub mechanism of the internal clock. ## Changes The core of the change is the `timesync` package, the rest of the PR is adapting the existing code to the new interface of the clock. `AwaitLayer` and `Subscribe` have the same purpose, but behave differently. This PR removes the duplicated code and simplifies the clock: - `Subscribe`/`Unsubscribe` and all logic associated with these two methods has been removed - `Ticker` and `TimeClock` have been merged into `NodeClock` - Methods `GetGenesisTime` and `GetCurrentLayer` have been renamed to `GenesisTime` and `CurrentLayer`. - The clock itself is now ticking once at the beginning of a layer and adjusts the waiting period until the next layer based on when the tick was issued. ## Test Plan Tests for `NodeClock` have been written from scratch, with emphasis on safe concurrent use and stability against time shifts that might be caused by NTP or the node hibernating. For this I added a dependency on http://github.com/benbjohnson/clock (was already an indirect dependency). It allows to mock time more easily in tests than it would be possible with `gomock`. ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot
pushed a commit
that referenced
this pull request
Feb 5, 2023
## Motivation Closes #4010, this is a follow up to #4012, #4013, #4014 and removes the now obsolete pub/sub mechanism of the internal clock. ## Changes The core of the change is the `timesync` package, the rest of the PR is adapting the existing code to the new interface of the clock. `AwaitLayer` and `Subscribe` have the same purpose, but behave differently. This PR removes the duplicated code and simplifies the clock: - `Subscribe`/`Unsubscribe` and all logic associated with these two methods has been removed - `Ticker` and `TimeClock` have been merged into `NodeClock` - Methods `GetGenesisTime` and `GetCurrentLayer` have been renamed to `GenesisTime` and `CurrentLayer`. - The clock itself is now ticking once at the beginning of a layer and adjusts the waiting period until the next layer based on when the tick was issued. ## Test Plan Tests for `NodeClock` have been written from scratch, with emphasis on safe concurrent use and stability against time shifts that might be caused by NTP or the node hibernating. For this I added a dependency on http://github.com/benbjohnson/clock (was already an indirect dependency). It allows to mock time more easily in tests than it would be possible with `gomock`. ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot
pushed a commit
that referenced
this pull request
Feb 5, 2023
## Motivation Closes #4010, this is a follow up to #4012, #4013, #4014 and removes the now obsolete pub/sub mechanism of the internal clock. ## Changes The core of the change is the `timesync` package, the rest of the PR is adapting the existing code to the new interface of the clock. `AwaitLayer` and `Subscribe` have the same purpose, but behave differently. This PR removes the duplicated code and simplifies the clock: - `Subscribe`/`Unsubscribe` and all logic associated with these two methods has been removed - `Ticker` and `TimeClock` have been merged into `NodeClock` - Methods `GetGenesisTime` and `GetCurrentLayer` have been renamed to `GenesisTime` and `CurrentLayer`. - The clock itself is now ticking once at the beginning of a layer and adjusts the waiting period until the next layer based on when the tick was issued. ## Test Plan Tests for `NodeClock` have been written from scratch, with emphasis on safe concurrent use and stability against time shifts that might be caused by NTP or the node hibernating. For this I added a dependency on http://github.com/benbjohnson/clock (was already an indirect dependency). It allows to mock time more easily in tests than it would be possible with `gomock`. ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot
pushed a commit
that referenced
this pull request
Feb 5, 2023
## Motivation Closes #4010, this is a follow up to #4012, #4013, #4014 and removes the now obsolete pub/sub mechanism of the internal clock. ## Changes The core of the change is the `timesync` package, the rest of the PR is adapting the existing code to the new interface of the clock. `AwaitLayer` and `Subscribe` have the same purpose, but behave differently. This PR removes the duplicated code and simplifies the clock: - `Subscribe`/`Unsubscribe` and all logic associated with these two methods has been removed - `Ticker` and `TimeClock` have been merged into `NodeClock` - Methods `GetGenesisTime` and `GetCurrentLayer` have been renamed to `GenesisTime` and `CurrentLayer`. - The clock itself is now ticking once at the beginning of a layer and adjusts the waiting period until the next layer based on when the tick was issued. ## Test Plan Tests for `NodeClock` have been written from scratch, with emphasis on safe concurrent use and stability against time shifts that might be caused by NTP or the node hibernating. For this I added a dependency on http://github.com/benbjohnson/clock (was already an indirect dependency). It allows to mock time more easily in tests than it would be possible with `gomock`. ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot
pushed a commit
that referenced
this pull request
Feb 5, 2023
## Motivation Closes #4010, this is a follow up to #4012, #4013, #4014 and removes the now obsolete pub/sub mechanism of the internal clock. ## Changes The core of the change is the `timesync` package, the rest of the PR is adapting the existing code to the new interface of the clock. `AwaitLayer` and `Subscribe` have the same purpose, but behave differently. This PR removes the duplicated code and simplifies the clock: - `Subscribe`/`Unsubscribe` and all logic associated with these two methods has been removed - `Ticker` and `TimeClock` have been merged into `NodeClock` - Methods `GetGenesisTime` and `GetCurrentLayer` have been renamed to `GenesisTime` and `CurrentLayer`. - The clock itself is now ticking once at the beginning of a layer and adjusts the waiting period until the next layer based on when the tick was issued. ## Test Plan Tests for `NodeClock` have been written from scratch, with emphasis on safe concurrent use and stability against time shifts that might be caused by NTP or the node hibernating. For this I added a dependency on http://github.com/benbjohnson/clock (was already an indirect dependency). It allows to mock time more easily in tests than it would be possible with `gomock`. ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot
pushed a commit
that referenced
this pull request
Feb 6, 2023
## Motivation Closes #4010, this is a follow up to #4012, #4013, #4014 and removes the now obsolete pub/sub mechanism of the internal clock. ## Changes The core of the change is the `timesync` package, the rest of the PR is adapting the existing code to the new interface of the clock. `AwaitLayer` and `Subscribe` have the same purpose, but behave differently. This PR removes the duplicated code and simplifies the clock: - `Subscribe`/`Unsubscribe` and all logic associated with these two methods has been removed - `Ticker` and `TimeClock` have been merged into `NodeClock` - Methods `GetGenesisTime` and `GetCurrentLayer` have been renamed to `GenesisTime` and `CurrentLayer`. - The clock itself is now ticking once at the beginning of a layer and adjusts the waiting period until the next layer based on when the tick was issued. ## Test Plan Tests for `NodeClock` have been written from scratch, with emphasis on safe concurrent use and stability against time shifts that might be caused by NTP or the node hibernating. For this I added a dependency on http://github.com/benbjohnson/clock (was already an indirect dependency). It allows to mock time more easily in tests than it would be possible with `gomock`. ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot
pushed a commit
that referenced
this pull request
Feb 6, 2023
## Motivation Closes #4010, this is a follow up to #4012, #4013, #4014 and removes the now obsolete pub/sub mechanism of the internal clock. ## Changes The core of the change is the `timesync` package, the rest of the PR is adapting the existing code to the new interface of the clock. `AwaitLayer` and `Subscribe` have the same purpose, but behave differently. This PR removes the duplicated code and simplifies the clock: - `Subscribe`/`Unsubscribe` and all logic associated with these two methods has been removed - `Ticker` and `TimeClock` have been merged into `NodeClock` - Methods `GetGenesisTime` and `GetCurrentLayer` have been renamed to `GenesisTime` and `CurrentLayer`. - The clock itself is now ticking once at the beginning of a layer and adjusts the waiting period until the next layer based on when the tick was issued. ## Test Plan Tests for `NodeClock` have been written from scratch, with emphasis on safe concurrent use and stability against time shifts that might be caused by NTP or the node hibernating. For this I added a dependency on http://github.com/benbjohnson/clock (was already an indirect dependency). It allows to mock time more easily in tests than it would be possible with `gomock`. ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
bors bot
pushed a commit
that referenced
this pull request
Feb 6, 2023
## Motivation Closes #4010, this is a follow up to #4012, #4013, #4014 and removes the now obsolete pub/sub mechanism of the internal clock. ## Changes The core of the change is the `timesync` package, the rest of the PR is adapting the existing code to the new interface of the clock. `AwaitLayer` and `Subscribe` have the same purpose, but behave differently. This PR removes the duplicated code and simplifies the clock: - `Subscribe`/`Unsubscribe` and all logic associated with these two methods has been removed - `Ticker` and `TimeClock` have been merged into `NodeClock` - Methods `GetGenesisTime` and `GetCurrentLayer` have been renamed to `GenesisTime` and `CurrentLayer`. - The clock itself is now ticking once at the beginning of a layer and adjusts the waiting period until the next layer based on when the tick was issued. ## Test Plan Tests for `NodeClock` have been written from scratch, with emphasis on safe concurrent use and stability against time shifts that might be caused by NTP or the node hibernating. For this I added a dependency on http://github.com/benbjohnson/clock (was already an indirect dependency). It allows to mock time more easily in tests than it would be possible with `gomock`. ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [x] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Part of #4010. Refactor code to not subscribe to layer changes but rather await new layers.
Changes
Use
AwaitLayerinBeaconinstead of a layer timer and make sure that the correct layer is processed byProtocolDriver:AwaitLayermight return late (e.g. if the node was hibernating while it was waiting for the next epoch) in this caseProtocolDriverwill continue with the current epoch rather than the one it was awaiting.GetCurrentLayermight return a layer lower than the one awaited (if a time sync happens right between awaiting and getting the current layer). In this case wait a little longer.listenEpochsnow fires once per epoch instead of once per layer and then checking if the layer is the first in an epoch. This should make the code a bit more efficient.Test Plan
n/a
TODO
DevOps Notes