feat: implemention of KLIP-13 #4099
Conversation
|
@confluentinc It looks like @alex-dukhno just signed our Contributor License Agreement. 👍 Always at your service, clabot |
There was a problem hiding this comment.
Hello @alex-dukhno - this looks pretty good to me! I don't think any documentation or integration tests need to be added to this, but if you could show the output of a LIST PROPERTIES on a server that has a connect worker running (copy and paste it into the testing done section) that would help confirm that everything works as expected.
|
@agavra I've updated PR description. |
There was a problem hiding this comment.
I didn't think about this when reviewing the KLIP (so thanks for bearing with me!), but looking at this now I just noticed that there are some overlaps (i.e. both connect and ksql specify bootstrap.servers)...
I think to resolve this we need to introduce "SCOPE" in this PR. It shouldn't be too tough, we can change PropertiesList to have an internal class:
@JsonIgnorePropertes(ignoreUnknown = true)
static class Property {
@JsonCreator
public Property(
@JsonProperty("property") final String property,
@JsonProperty("scope") final String scope
}And then the properties field can be a Map<Property, ?> instead of Map<String, ?> and can now have duplicate property names under different scope. Similarly, we would change PropertiesListTableBuilder to include the extra column.
|
@agavra I have added Just let me know if I did something silly 😛. Especially, tests fixes feels like "ad-hoc" solutions. |
| + " k1 | KSQL | SERVER | 1 \n" | ||
| + " k2 | KSQL | SERVER | v2 \n" | ||
| + " k3 | KSQL | SERVER | true \n" | ||
| + "-------------------------------------------------------\n")); |
| + " \"k3\" : true\n" | ||
| + " \"k3-KSQL\" : true,\n" | ||
| + " \"k2-KSQL\" : \"v2\",\n" | ||
| + " \"k1-KSQL\" : 1\n" |
There was a problem hiding this comment.
🤔 yet again, I missed something with my suggestion - I forgot that JSON can't represent objects as map keys! The problem with this is that if anyone is depending on our HTTP API without having access to the java JARs deserializing this would be a little though. I apologize for the back and forth, but looking at this I think we need to switch up the model a little bit.
Instead, what do you think about modeling it as a map from property to a list of (scope, value) objects? That way, the JSON API would return:
"properties": {
"bootstrap.servers": [
{"scope": "KSQL", "value": "foo1"},
{"scope": "EMBEDDED CONNECT WORKER", "value": "foo2"}
]
}let me get the opinion of some other ksqlDB committers about this (it could be that the current solution is OK)
There was a problem hiding this comment.
Alternatively, we can just return a list of Property instead of a map where property contains the name, value and scope (@big-andy-coates for the idea)!
There was a problem hiding this comment.
we can just return a list of Property instead of a map where property contains the name, value and scope
@agavra I made required changes, could you please take a look? Thank you
Introduce KSQL command to print connect worker properties to the console
alex-dukhno
force-pushed
the
klip-impl-13-introduce-KSQL-command-to-print-connect-worker-properties-to-the-console
branch
from
795d922
to
256e323
Compare
There was a problem hiding this comment.
LGTM! It looks like checkstyle is failing (see inline comment). After that works there may be some other test failures (mvn clean install should catch all of them). I'll merge it when the build is green.
Thanks @alex-dukhno, this is great feature 😄
| return "Property{" + | ||
| "name='" + name + '\'' + | ||
| ", scope='" + scope + '\'' + | ||
| ", value='" + value + '\'' + | ||
| '}'; |
There was a problem hiding this comment.
nit: our checkstyle requires + to be on the new line instead of at the end:
new "Property{"
+ "name=" + name + '\'' ...
|
@agavra I fix the checkstyle issue and some tests that failed after running Thank you for your code reviews |
|
Thanks for the contribution @alex-dukhno! I've merged this PR. |
alex-dukhno
deleted the
klip-impl-13-introduce-KSQL-command-to-print-connect-worker-properties-to-the-console
branch
Description
Implementation of KLIP-13 Introduce KSQL command to print connect worker properties to the console
(LIST|SHOW) PROPERTIEScommand is extended to print embedded connect worker propertiesTesting done
N/A
Do we need integration test that touch file system to read connect worker properties file, or it would be better hide the fact that its properties come from file system?
Edited:
This is how it looks in the terminal on my machine
Documentation
I couldn't find the documentation part that should be changed. Is there another repo for that part?
Reviewer checklist