Skip to content

Conversation

@mattnibs
Copy link
Collaborator

@mattnibs mattnibs commented Feb 1, 2024

This commit adds the ability to run zed manage in the same process as the zed serve command. If the -manage flag is set with a duration greater than 0 manage will run.

This commit also move the manage package to lake/api/manage package and simplifies the logic for the monitor function.

@mattnibs mattnibs force-pushed the serve-manage branch 2 times, most recently from c310689 to 9e60962 Compare February 1, 2024 17:26
This commit adds the ability to run zed manage in the same process as the
zed serve command. If the -manage flag is set with a duration greater
than 0 manage will run.

This commit also move the manage package to lake/api/manage package and
simplifies the logic for the monitor function.
Copy link
Member

@nwt nwt left a comment

Choose a reason for hiding this comment

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

I'd like to have @philrz give this a spin before we merge it.

@mattnibs
Copy link
Collaborator Author

mattnibs commented Feb 6, 2024

I'd like to have @philrz give this a spin before we merge it.

Agreed.

@mattnibs mattnibs requested a review from philrz February 6, 2024 18:15
@philrz
Copy link
Contributor

philrz commented Feb 7, 2024

I just tested this branch out at commit 9ce0f4f and it seems to work fine. One thing I found a little odd though is that I went looking for a ztest in this PR since that usually gives me a starting point for seeing how the new functionality is invoked and it looks like the only ztest change is to something that only exercises the pre-existing approach that used the zed manage approach. Does the continuous nature of zed serve -manage make it too difficult to hit with ztest?

@mattnibs
Copy link
Collaborator Author

mattnibs commented Feb 7, 2024

@philrz yes

@philrz philrz mentioned this pull request Feb 7, 2024
@philrz
Copy link
Contributor

philrz commented Feb 7, 2024

After an offline discussion with @nwt he agreed to take up the testing angle separately/later via what's now tracked in #5024.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants