Skip to content

feat(kafka topic): add topic commands#309

Merged
craicoverflow merged 6 commits intomainfrom
topic-commands
Feb 16, 2021
Merged

feat(kafka topic): add topic commands#309
craicoverflow merged 6 commits intomainfrom
topic-commands

Conversation

@craicoverflow
Copy link
Contributor

@craicoverflow craicoverflow commented Feb 1, 2021

This PR contains changes related to kafka topic subcommands.
This is a work in progress branch that should not get merged until the data plane UI server is live on api.stage.openshift.com.

Running this branch

This branch uses a mocked server hosting in-memory topics. To run the mock server run make mock-api/start. You can see the results API data at http://localhost:8001/topics.

Now you can run rhoas kafka topic list etc.

Tasks

  • Add mock server
  • Add topic list command
  • Add topic create command
  • Add topic update command
  • Add topic get command
  • Add topic delete command
  • Add topic deletion confirmation dialogue
  • Remove hard-coded call to mock server in KeycloakConnection

Epic Stories

I want create a topic, defining its name and , optionally number of partitions , replication factor and message retention

Run rhoas kafka topic create topic-4 --partitions 2 --replicas 1 --retention-ms -1 to set a custom partition count. The default value is 1.

update the number of partitions configuration of my topic

You can pass --partitions to update the number of partitions. The partition number cannot be decreased.

Example: rhoas kafka topic update topic-4 --replicas 3

I wan to update the topic replication factor

rhoas kafka topic update topic-4 --replicas 3

update the topic message retention

rhoas kafka topic update topic-4 --retention-ms -1

delete an existing topic from my Kafka cluster, including all its messages

rhoas kafka topic delete topic-4

describe a topic and see the detailed configuration of my Kafka topic

rhoas kafka topic topic-4 describe

@wtrocki
Copy link
Collaborator

wtrocki commented Feb 1, 2021

CC @dlabaj - Enda is doing some mocking for the server. It is worth to collaborate on this

@craicoverflow craicoverflow changed the title wip: mock topics server wip: topics Feb 1, 2021
@craicoverflow craicoverflow changed the title wip: topics [wip] topics Feb 1, 2021
@craicoverflow
Copy link
Contributor Author

@dlabaj in case you were interested - I have written some mock handlers from Strimzi API mock here

https://github.com/bf2fc6cc711aee1a0c2a/cli/blob/topic-commands/mas-mock/src/strimzi-api-handlers.js

@craicoverflow craicoverflow force-pushed the topic-commands branch 4 times, most recently from ba4afa3 to dcd56e5 Compare February 3, 2021 14:49
@craicoverflow craicoverflow requested a review from wtrocki February 3, 2021 14:50
@craicoverflow
Copy link
Contributor Author

craicoverflow commented Feb 3, 2021

Hey @wtrocki this is ready for your review. Please do not merge as it is still relying on the mock API.

@craicoverflow craicoverflow changed the title [wip] topics topics Feb 3, 2021
@craicoverflow
Copy link
Contributor Author

Sorry, need to add a confirmation dialogue for deleting a topic.

@craicoverflow
Copy link
Contributor Author

Ready for review again.

// start server
app.listen(8000, () => console.info("api listening at http://localhost:8000"));
strimziApiServer.use((req, res) => topicAPI.handleRequest(req, req, res))
.listen(8001, () => console.info("Strimzi Admin API listening at http://localhost:8001")) No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

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

How this works? 2 diffrent ports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See https://github.com/bf2fc6cc711aee1a0c2a/cli/blob/topic-commands/pkg/connection/keycloak_connection.go#L175-L177

I decided to hard-code the URL to the API for now as I could not get any of the solutions we discussed to work. This is why this PR should not be merged until the data plane API goes live.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right. Any reason to not combine those API or use different middlewares?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not resolve the path issue we were chatting about the other day and it was blocking me from proceeding with the topic commands work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

SO this will be resolved when merging or will requires some extra changes in code to test?
Let's sync tomorrow on this (short call)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO this PR should not be merged until the API is available on staging because the current topic commands work (somewhat) and are useful to test connecting to the data plane. The new commands only work with a mock environment. Once the API is ready code will be adjusted with original API base url.

If I get time I can try and resolve the URL issue but i have other priorities too and do not want to spend too long on it.

@wtrocki
Copy link
Collaborator

wtrocki commented Feb 8, 2021

Verified with the mock

@craicoverflow
Copy link
Contributor Author

Updated guides

@craicoverflow craicoverflow force-pushed the topic-commands branch 2 times, most recently from 7655765 to 104cc25 Compare February 16, 2021 09:12
@craicoverflow craicoverflow linked an issue Feb 16, 2021 that may be closed by this pull request
@craicoverflow
Copy link
Contributor Author

This is now at 109 files..needs to be merged as keeping it up to date with master is a huge burden. Disabling the topics commands on master.

@craicoverflow craicoverflow changed the title topics feat(kafka topic): add topic commands Feb 16, 2021
@craicoverflow craicoverflow merged commit 9e399f5 into main Feb 16, 2021
@craicoverflow craicoverflow deleted the topic-commands branch February 16, 2021 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants