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

Improve output for topic changes on dryRun and apply #295

Closed
akselh opened this issue Jun 16, 2021 · 10 comments · Fixed by #383
Closed

Improve output for topic changes on dryRun and apply #295

akselh opened this issue Jun 16, 2021 · 10 comments · Fixed by #383
Labels
enhancement New feature or request

Comments

@akselh
Copy link
Contributor

akselh commented Jun 16, 2021

There have been multiple other issues (#220, #102, #109) on this subject and also a PR that got closed. This issue proposes a solution for the most pressing issues, including the need for a refactor of code related to this.

Is your feature request related to a problem? Please describe.

Functional issues with current solution

Currently JulieOps only logs planned changes during a dryRun execution. For topics the logging also has the following issues:

  • It is not possible to get a log of the actual topic changes for topic config updates.
    • The update topic action is performed regardless of any changes to topic configuration (topology vs cluster state). This is not a problem in the logging as such, but the fact that Julie always updates config for all topics in the topology (apply phase) might be confusing.
  • Similarly there is no logging of what config is set for a new topic
  • It is difficult to get an overview of new vs updated topics, because there is no ordering of action execution. Create and update topic action exections are intermingled and so is the logging.

For bindings (ACLs/RBAC etc) the logging is correct as the actual new/changed ACLs is updated and logged. However there are also a corresponding issue that currently there is no logging of the updates done on schemas.

In addition the apply phase does not log the changes that have actually been performed; only a listing of all topics and all ACLs in the cluster.

Technical issues with current solution

  • SyncTopicAction is doing too many things:
    • Creating new topics
    • Updating topic config
    • Adding schemas to Schema Registry
  • Wording is misleading for some core methods. For example TopicManager.apply indicates that the manager will apply the supplied ExecutionPlan, but rather actions is added to the plan.
  • The classes for building of bindings (for later update to ACLs etc) is modeled as subclasses of BaseAction although they are not actions - only creating the bindings that will later be used by actions.
  • Interface ManagerOfThings: Naming is not very descriptive. Should also be implemented by all managers if a common interface is needed.
  • Would be better to handle printing of the execution log in another way than just doing toString on the actions. Maybe re-design to facilitate different outputs? JSON vs more compact humen readable?

Describe the solution you'd like

Proposal for functional changes

  • Execution plan logging for topic is changed to log the actual config changes (topology vs cluster state)
    • So only topics with actual changes to the config will be updated towards the cluster
  • Logging/execution of new vs updated topics is ordered, with new topic being logged first
    • Also changing the ordering of the actual executions
  • For apply phase also print the execution log -> what have been changed in the cluster

Example of new logs for topic and schemas changes:

{
  "Operation" : "com.purbon.kafka.topology.actions.topics.CreateTopicAction",
  "Topic" : "schemas.json.foo.bar.avro",
  "Action" : "create"
}

{
  "Operation" : "com.purbon.kafka.topology.actions.topics.UpdateTopicConfigAction",
  "Topic" : "testTopicConfigUpdate-test.project.topicA",
  "Action" : "update",
  "Changes" : {
    "NewConfigs" : {
      "min.insync.replicas": "2"
    },
    "UpdatedConfigs" : {
      "retention.bytes" : "104"
    },
    "DeletedConfigs" : {
      "segment.bytes" : "104857600"
    },
   "UpdatedPartitionCount": "3"
  }
}

{
  "Operation" : "com.purbon.kafka.topology.actions.topics.DeleteTopics",
  "topics" : [ "testTopicCreationWithChangedTopology.project.topicB", "testTopicCreationWithChangedTopology.project.topicA" ]
}

{
  "Operation" : "com.purbon.kafka.topology.actions.topics.RegisterSchemaAction",
  "Topic" : "schemas.avro.foo.cat.avro",
  "Schemas" : {
    "schemas.avro.foo.cat.avro-key" : "schemas/bar-key.avsc",
    "schemas.avro.foo.cat.avro-value" : "schemas/bar-value.avsc"
  }
}

Proposal for technical changes

Some refactoring is required to support the functional improvements:

  • Split SyncTopicAction into CreateTopicAction, SyncTopicConfigAction and RegisterSchemaAction.
  • Refactoring of how updates on config is done: First build a model of the actual changes to be done (i.e. what configs have actually changed), then give that model to the action. That model should also be basis for execution plan/log.
  • Refactoring of how execution plan/log is printed
  • Redesign BuildBindings-classes, should not be actions
  • Renaming of ManagerOfThings and interface methods
@akselh akselh added the enhancement New feature or request label Jun 16, 2021
@akselh
Copy link
Contributor Author

akselh commented Jun 16, 2021

Hi @purbon , I hope you are all good!

There is already have work-in-progress on parts of this so if possible would be nice with some feedback on what you think :-)

@sverrehu
Copy link
Contributor

@purbon, it would be very helpful if you replied to this. We have several quite large changes in our local pipeline now, to support Aksel's suggestions.

@purbon
Copy link
Collaborator

purbon commented Jun 20, 2021 via email

@akselh
Copy link
Contributor Author

akselh commented Jun 21, 2021

@purbon , no problem. I fully understand. Yes, a call to discuss and agree on the changes would be good. We can agree on time and channel via Gitter?

@purbon
Copy link
Collaborator

purbon commented Jun 21, 2021 via email

@akselh
Copy link
Contributor Author

akselh commented Jun 21, 2021

👍 Gitter message sent.

@akselh
Copy link
Contributor Author

akselh commented Jun 23, 2021

Hi, sorry guys ... really last weeks been super busy. I propose one thing, do you want to do 1h call this week? we could probably arrange and close everything on that call. What do you think? Missatge de Sverre H. Huseby @.***> del dia ds., 19 de juny 2021 a les 2:34:

@purbon https://github.com/purbon, it would be very helpful if you replied to this. We have several quite large changes in our local pipeline now, to support Aksel's suggestions. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#295 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAQXPFV2SNEUX3VNCEKJDTTTPQZBANCNFSM46ZIPPSQ .
-- Pere Urbon-Bayes Software Architect https://twitter.com/purbon https://www.linkedin.com/in/purbon/

Hi @purbon, when can we have the call?

@peteryongzhong
Copy link

Any update on this issue. I think it would be extremely helpful to have this feature.

@akselh
Copy link
Contributor Author

akselh commented Jul 12, 2021

I have not gotten any feedback from @purbon, but we have started work on a PR. Right now we have entered summer holidays, will pick it up again in August. 🌞😎

@akselh
Copy link
Contributor Author

akselh commented Aug 10, 2021

Hi @purbon, hope summer has been good! Still on summer vacation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants