-
Notifications
You must be signed in to change notification settings - Fork 135
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
Add TKS TPSConnectorService to v2 APIs #4822
Conversation
1468004
to
34257e5
Compare
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
String host = request.getParameter("host"); | ||
String port = request.getParameter("port"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note, we probably should consider requiring the user to specify the connector ID so that we don't have to generate it. The current findNextConnectorID()
might not generate consistent IDs across the cluster which could be problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the current implementation of findNextConnectorID()
does not work across a cluster. The user could provide the ID or we have to implement a way to get the information consistent in a cluster scenario. Do we create a ticket for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a design issue, not just the API, but also the storage, which might not be trivial to fix. So far there's nobody asking to fix it so I'd just leave it as is for now. Maybe in the future when we fully move to containers this issue will come up again and we can redesign at that point.
@edewata Thanks! |
No description provided.