Skip to content

Conversation

@skyzh
Copy link
Contributor

@skyzh skyzh commented Mar 21, 2022

Signed-off-by: Alex Chi [email protected]

What's changed and what's your intention?

As frontend ./risedev p cannot start for now (see #1093), I'd like to compare the generated proto with the java frontend to ensure that create MV statements are producing nearly the same proto as before. Therefore, I added stream_proto test.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

@github-actions github-actions bot added the type/feature Type: New feature. label Mar 21, 2022
@skyzh skyzh requested a review from fuyufjh March 21, 2022 06:03
Signed-off-by: Alex Chi <[email protected]>
Copy link
Collaborator

@fuyufjh fuyufjh left a comment

Choose a reason for hiding this comment

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

LGTM for rest


// Only generate batch_plan if it is specified in test case
if self.batch_plan.is_some() {
ret.batch_plan = Some(explain_plan(&batch_plan));
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC, the proto of batch plan contains the information of exchange source (node IP & port), so it's not possible to test it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We always run it with our own test runner, so we can get a consistent result by manually setting the IPs when creating the mock compute nodes...

@skyzh skyzh enabled auto-merge (squash) March 21, 2022 06:19
@skyzh skyzh merged commit 89d5f08 into main Mar 21, 2022
@skyzh skyzh deleted the skyzh/add-stream-proto-test branch March 21, 2022 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/feature Type: New feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants