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

Service Beta #813

Merged
merged 30 commits into from
Dec 23, 2022
Merged

Service Beta #813

merged 30 commits into from
Dec 23, 2022

Conversation

scottf
Copy link
Contributor

@scottf scottf commented Dec 8, 2022

No description provided.

@scottf scottf changed the title Service Beta (Incomplete) Service Beta (Draft) Dec 8, 2022
@scottf scottf changed the title Service Beta (Draft) Service Beta Dec 12, 2022
@scottf scottf marked this pull request as ready for review December 12, 2022 20:54
src/main/java/io/nats/service/ServiceUtil.java Outdated Show resolved Hide resolved
src/main/java/io/nats/service/ServiceUtil.java Outdated Show resolved Hide resolved
src/main/java/io/nats/service/api/EndpointStats.java Outdated Show resolved Hide resolved
src/main/java/io/nats/service/api/Schema.java Outdated Show resolved Hide resolved
src/main/java/io/nats/service/api/StatsResponse.java Outdated Show resolved Hide resolved
Copy link
Member

@aricart aricart left a comment

Choose a reason for hiding this comment

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

Looks good to me - with the exception of the nit on the name of a parameter. The other observation just needs to be checked, I didn't find where the values are uppercased.

public static final Duration DEFAULT_DRAIN_TIMEOUT = Duration.ofSeconds(5);
public static final long DEFAULT_DISCOVERY_MAX_TIME_MILLIS = 5000;
public static final int DEFAULT_DISCOVERY_MAX_RESULTS = 10;

Copy link
Member

Choose a reason for hiding this comment

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

baseSubject is your action above, why not call it the same

Copy link
Member

Choose a reason for hiding this comment

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

Also with the exception of the prefix - if one is given to straddle account boundaries, all other tokens (verb, name, id) should be uppercase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, prefix replacement is unresolved so I wouldn't worry about his for right now. It will be addressed when the decision about how to modify the prefix for across account is made. Will it be done KV/Object Store style or will it be done JS Prefix style?

@aricart
Copy link
Member

aricart commented Dec 16, 2022

I am writing a tool that you can use to verify that your service is cross-implementation compatible with respect to the monitoring apis

@scottf scottf merged commit 253cdc0 into main Dec 23, 2022
@scottf scottf deleted the service-beta branch December 23, 2022 13:09
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.

None yet

8 participants