/cockroach

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

Changing NewInMem to have storeSize and setting cacheSize to 0 #16876

Open
wants to merge 1 commit into
base: master
from

Conversation

Reviewers

@cockroachdb/core-prs core-prs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@soniabhishek @cockroach-teamcity @bdarnell @tschottdorf
@soniabhishek
Contributor

soniabhishek commented Jul 6, 2017 •
edited

fixes #16750
what else need to be done in this PR?
@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Jul 6, 2017

Member

This change is Reviewable
Member

cockroach-teamcity commented Jul 6, 2017

This change is Reviewable
@bdarnell

This comment has been minimized.

Show comment
Hide comment
@bdarnell

bdarnell Jul 6, 2017

Member

:lgtm:

I think this is all we need, but we're having some issues with our CI builds right now. When #16879 is merged you'll need to rebase on top of it.

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable
Member

bdarnell commented Jul 6, 2017

:lgtm:

I think this is all we need, but we're having some issues with our CI builds right now. When #16879 is merged you'll need to rebase on top of it.

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

@benesch benesch referenced this pull request Jun 21, 2018

Merged

storage: shore up range merges #25160

@tschottdorf

This comment has been minimized.

Show comment
Hide comment
@tschottdorf

tschottdorf Jun 25, 2018

Member

Friendly ping.
Member

tschottdorf commented Jun 25, 2018

Friendly ping.
@soniabhishek

This comment has been minimized.

Show comment
Hide comment
@soniabhishek

soniabhishek Jun 26, 2018

Contributor

Oh sorry, I forgot! Is this still open and can I merge this @tschottdorf
Contributor

soniabhishek commented Jun 26, 2018

Oh sorry, I forgot! Is this still open and can I merge this @tschottdorf
@soniabhishek @tschottdorf
storage: fix parameter to NewInMem to size the store, not the cache 
Fixes #16750.

Release note: None

Release note: None
Loading status checks…
512c639

@soniabhishek soniabhishek requested a review from cockroachdb/core-prs as a code owner Oct 11, 2018

@tschottdorf

This comment has been minimized.

Show comment
Hide comment
@tschottdorf

tschottdorf Oct 11, 2018

Member

Fixed up the commit for you. Thanks @soniabhishek and sorry about letting this sit for so long.

bors r+
Member

tschottdorf commented Oct 11, 2018

Fixed up the commit for you. Thanks @soniabhishek and sorry about letting this sit for so long.

bors r+

craig bot pushed a commit that referenced this pull request Oct 11, 2018

craig[bot] @soniabhishek
Merge #16876 
16876: Changing NewInMem to have storeSize and setting cacheSize to 0 r=tschottdorf a=soniabhishek

fixes #16750
what else need to be done in this PR?

Co-authored-by: Abhishek Soni <abhishek.rocks26@gmail.com>
Loading status checks…
849858d
@craig

This comment has been minimized.

Show comment
Hide comment
@craig

craig bot Oct 11, 2018

Build failed

craig bot commented Oct 11, 2018

Build failed
@tschottdorf

This comment has been minimized.

Show comment
Hide comment
@tschottdorf

tschottdorf Oct 11, 2018

Member

Wow, this really blew up CI. Guess it needs more love.
Member

tschottdorf commented Oct 11, 2018

Wow, this really blew up CI. Guess it needs more love.
@soniabhishek

This comment has been minimized.

Show comment
Hide comment
@soniabhishek

soniabhishek Oct 11, 2018

Contributor

OOPS! a lot of failures, what might have been the issue @tschottdorf interested in fixing this.
Contributor

soniabhishek commented Oct 11, 2018

OOPS! a lot of failures, what might have been the issue @tschottdorf interested in fixing this.
@tschottdorf

This comment has been minimized.

Show comment
Hide comment
@tschottdorf

tschottdorf Oct 11, 2018

Member

I'm not sure. I would've said that changing the cache size or applying the store size some tests pass is messing things up, but somehow the test passed under race (which is usually where things that break under regular test break as well).

Does make test pass locally for you?
Member

tschottdorf commented Oct 11, 2018

I'm not sure. I would've said that changing the cache size or applying the store size some tests pass is messing things up, but somehow the test passed under race (which is usually where things that break under regular test break as well).

Does make test pass locally for you?
@soniabhishek

This comment has been minimized.

Show comment
Hide comment
@soniabhishek

soniabhishek Oct 11, 2018

Contributor

Will have to check again! (Long time)
Contributor

soniabhishek commented Oct 11, 2018

Will have to check again! (Long time)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment