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.

Sign up for GitHub

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

Jump to bottom

emulator: Fix MZ_SEGMENT_CLIENT_SIDE #30964

Merged
merged 1 commit into from
Jan 7, 2025
Merged

emulator: Fix MZ_SEGMENT_CLIENT_SIDE #30964

merged 1 commit into from
Jan 7, 2025

Conversation

def-
Copy link
Contributor

@def- def- commented Jan 7, 2025 •
edited
Loading

Emulator (docker run -p 127.0.0.1:6875:6875 -p 127.0.0.1:6876:6876 -p 127.0.0.1:6877:6877 materialize/materialized:latest) currently fails to come up:

error: invalid value 'hMWi3sZ17KFMjn2sPWo9UJGpOQqiba4A' for '--segment-client-side'
  [possible values: true, false]

This is awkward enough that we might consider putting this into the current release, thus marking as a blocker preliminarily.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@def- def- requested review from benesch and bosconi January 7, 2025 17:31
@def- def- added the release-blocker Critical issue that should block *any* release if not fixed label Jan 7, 2025
@def-
Copy link
Contributor Author

def- commented Jan 7, 2025 •
edited
Loading

I believe this regressed because of #30919 CC @alex-hunt-materialize
Previously the invalid value was apparently just interpreted as true.

v0.127.2 was still fine.

@def- def- enabled auto-merge January 7, 2025 17:40
@def-
Copy link
Contributor Author

def- commented Jan 7, 2025

I'll add a test later this week to make sure the emulator defaults work, we shouldn't have only caught this by chance.

@def- def- merged commit 2af6bbc into MaterializeInc:main Jan 7, 2025
69 of 71 checks passed
@def- def- deleted the pr-fix-emulator branch January 7, 2025 18:22
def- added a commit to def-/materialize that referenced this pull request Jan 8, 2025
@def-
tests: Add simplest possible emulator test
5e0bea7 
Regression test for MaterializeInc#30964

This is only to make sure we don't shoot ourselves into the foot by
setting non-functional defaults.
@def- def- mentioned this pull request Jan 8, 2025
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex-hunt-materialize alex-hunt-materialize alex-hunt-materialize approved these changes

@benesch benesch Awaiting requested review from benesch

@bosconi bosconi Awaiting requested review from bosconi

Assignees
No one assigned
Labels
release-blocker Critical issue that should block *any* release if not fixed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@def- @alex-hunt-materialize