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

Integrate explorer #603

Closed
wants to merge 8 commits into from
Closed

Conversation

al8n
Copy link
Contributor

@al8n al8n commented Mar 4, 2024

The test cases in this PR shows that the explorer can work as expectedly.

@al8n al8n self-assigned this Mar 4, 2024
@al8n al8n added the enhancement New feature or request label Mar 4, 2024
@al8n al8n changed the title WIP: Integration for explorer Integration for explorer Mar 11, 2024
@al8n al8n changed the title Integration for explorer Integrate explorer Mar 11, 2024
Comment on lines +4 to +7
"-C",
"link-args=-framework CoreFoundation -framework Security -framework CoreServices -lresolv",
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may be changed by fmt tools, will change it back.

Comment on lines 149 to 156
match self
.carnot_tree
.committee_by_member_id(&id)
.map(|c| apply_threshold(c.len(), self.threshold))
.expect("node is not part of any committee")
{
Some(threshold) => threshold,
None => panic!("node {id} is not part of any committee"),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is there a change in the carnot tree in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this is used when I debugging, forget to change it back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still here. As well as other few changes and nitpicks.

Comment on lines 192 to 195
pub fn run_app_without_terminal(
&self,
username: String,
) -> Result<(std::sync::mpsc::Receiver<Vec<ChatMessage>>, App), Box<dyn std::error::Error>>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be used in the chat demo test, I do not upload that test case because of a wired "addr already in used" error.

Comment on lines +159 to +161
ONCE_INIT.call_once(move || {
registry_init!(layer, config.format, config.level);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the issue with the registry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running multiple Overwatch services with log service in one test case in different threads will lead to panic as tracing only allows setting global subscriber once. There is PR #577 for it, I will update that PR to feature gate this in test.

@danielSanchezQ
Copy link
Collaborator

There are failing checks. Overall looks ok.
Rocks db backend need to be merged first right?

@al8n al8n closed this Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants