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

Revise Config and fix various things #99

Merged
merged 3 commits into from
Aug 6, 2020
Merged

Conversation

junha1
Copy link
Contributor

@junha1 junha1 commented Aug 6, 2020

No description provided.

@junha1 junha1 requested a review from majecty August 6, 2020 03:15
@@ -70,7 +95,8 @@ impl Config {
Self {
name: "my rto".to_owned(),
call_slots: 512,
call_timeout: std::time::Duration::from_millis(1000),
maximum_services_num: 65536,
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it more common to 65535 as a maximum value rather than 65536?
I thought 65535 is better because it is u16::MAX https://doc.rust-lang.org/std/primitive.u16.html#associatedconstant.MAX
It is not an important comment. I want to know in which point 65536 is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I'll fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I thought about it, but it's common to use 2^N instead of 2^N-1 for a size of something, not index.

Comment on lines +66 to 72
/// Number of the maximum of concurrent calls.
///
/// Value of this doesn't have anything to do with the number of threads that would be spawned.
/// Having a large number of this wouldn't charge any cost except really small additional memory allocation.
///
/// [`server_threads`]: ./struct.Config.html#field.server_threads
pub call_slots: usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

If this value is not important, we may use a Vec to save slots and make it scale automatically(Or give an enough default value).
Providing a not important option makes the system difficult to use.
You don't need to fix it right now. Please consider 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.

Ok. Let's keep the parameters as raw as possible for now, for easy debug/test at least.
We might provide more high level interface to setup a parameter later

Comment on lines +82 to +83
/// A maximum number of services that this context can export.
pub maximum_services_num: usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's subtle to provide this as an option.

It may hard to predict what will be a good maximum service number.
I don't know what will be a good maximum_services_num value when I implement Foundry modules. However small maximum_services_num may make a deadlock or panic.

If we want to make a good library that is easy to use, it will be better the library manages these things in its code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention is make as many options as possible, for full control over the context. I think we can think about this later

@junha1 junha1 merged commit 04bbc0d into CodeChain-io:master Aug 6, 2020
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.

2 participants