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.

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

cleanup: Removing RuntimeConfigView -> RuntimeConfig #8763

Merged
merged 2 commits into from
Apr 25, 2023

Conversation

aborg-dev
Copy link
Contributor

@aborg-dev aborg-dev commented Mar 21, 2023

In #8426 this conversion became lossy due to the introduction of Compute Costs. This raised the question of whether this conversion is needed at all. I've found one place in the Indexer code that uses it and replaced it with a direct lookup for config from RuntimeConfigStore. As a part of this I've also simplified some code in the indexer.

This should fix the failing nayduck test #8967

@aborg-dev aborg-dev added the C-housekeeping Category: Refactoring, cleanups, code quality label Mar 21, 2023
@aborg-dev aborg-dev requested a review from jakmeier March 21, 2023 11:26
@aborg-dev aborg-dev marked this pull request as ready for review April 24, 2023 13:08
@aborg-dev aborg-dev requested a review from a team as a code owner April 24, 2023 13:08
@aborg-dev aborg-dev requested review from khorolets and removed request for jakmeier April 24, 2023 15:41
Copy link
Member

@khorolets khorolets left a comment

Choose a reason for hiding this comment

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

I hope this returns the same result for the older data 😅

@aborg-dev
Copy link
Contributor Author

I hope this returns the same result for the older data 😅

That sounds important :)
Is there any experiment I can run in order to verify this at least for some time point?

@near-bulldozer near-bulldozer bot merged commit d752d74 into near:master Apr 25, 2023
nikurt pushed a commit that referenced this pull request Apr 25, 2023
In #8426 this conversion became lossy due to the introduction of Compute Costs. This raised the question of whether this conversion is needed at all. I've found one place in the Indexer code that uses it and replaced it with a direct lookup for config from `RuntimeConfigStore`. As a part of this I've also simplified some code in the indexer.

This should fix the failing nayduck test #8967
nikurt pushed a commit that referenced this pull request Apr 28, 2023
In #8426 this conversion became lossy due to the introduction of Compute Costs. This raised the question of whether this conversion is needed at all. I've found one place in the Indexer code that uses it and replaced it with a direct lookup for config from `RuntimeConfigStore`. As a part of this I've also simplified some code in the indexer.

This should fix the failing nayduck test #8967
@aborg-dev aborg-dev deleted the remove_from_view branch June 12, 2023 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-housekeeping Category: Refactoring, cleanups, code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants