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

Add event_id field to Event base class #5

Merged
merged 2 commits into from
May 6, 2022
Merged

Conversation

GauthamGoli
Copy link
Contributor

No description provided.

@GauthamGoli GauthamGoli requested a review from sidmitra March 18, 2022 08:47
eventbusk/bus.py Outdated
@@ -31,6 +32,7 @@ class MyEvent(Event):
foo: int
bar: str
"""
event_id: str = field(default=False, init=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Opinion: Consider using default_factory=uuid.uuid4. More: https://docs.python.org/3/library/dataclasses.html#dataclasses.field and you can convert to string while sending.

Copy link
Contributor

@kracekumar kracekumar Mar 21, 2022

Choose a reason for hiding this comment

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

Opinion: I feel id is better name. Consider readability for event.id vs event.event_id

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be fine with id going forward. But not that adding this might cause other issues in existing events where id is used.

https://github.com/airbase/airbase-backend/blob/master/airbase_platform/receivers.py#L69-L70

I would assume the child classes can anyway override the base class definition? So we can figure out a transition for existing use cases if we need to do this.

cc @gman90 since @Airbase/vault will be one of the core users of kafka .

We can also get on a periodic call if needed, until we iron out some basics. I think there are a bunch of people that are starting out this quarter on the eventbus, and i've noticed some issues(which might or might not be something important).

Copy link
Contributor Author

@GauthamGoli GauthamGoli Mar 24, 2022

Choose a reason for hiding this comment

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

So we can figure out a transition for existing use cases if we need to do this.

For https://github.com/airbase/airbase-backend/blob/master/airbase_platform/receivers.py#L69-L70
Either

  1. Introduce a v2 event OR
  2. Add an additional state_id field (to replace idusage) at producer and add logic within consumer to handle this

Edit:
Taking a closer look, we'll have to use both the above approaches sequentially.
Attempting to move id field from subclasses to base Event class is unique case and different from any event migrations that we might see in future, this particular migration has to be done in 3 phases

  1. Introduce v2 event(s) with existing id field renamed, Implement V2 consumers
  2. Once existing V1 events are all consumed, Remove V1 events, v1 topics
  3. Refactor event_id to id in Base classes, update consumers and producers

Copy link
Contributor

@sidmitra sidmitra Mar 29, 2022

Choose a reason for hiding this comment

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

@GauthamGoli Anything is fine, as long as you ensure the existing topics don't break(or there's a workaround to fix issues)

Copy link

Choose a reason for hiding this comment

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

The main thing about changing the base class id field is what happens to inflight events. At this point, the event class is the schema so changing that will cause an issue for inflight events. We are going to have to think about mirgrations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussion with @sidmitra and @gman90 we'd decided to go forward with event_id field to avoid refactor and migration effort.

I've however updated Event base class to update type of event_id to uuid.UUID

@sidmitra sidmitra requested a review from gman90 March 22, 2022 00:49
@GauthamGoli GauthamGoli requested a review from kracekumar April 7, 2022 13:27
@sidmitra
Copy link
Contributor

sidmitra commented Apr 7, 2022

Merging this means this will automatically go live on production; because in pyproject.toml on backend we only specify branch.

eventbusk = {git = "https://github.com/Airbase/eventbusk.git", rev = "main"}

Maybe we should first specify a commit or tag and increase the existing version first and specify that on the backend. We don't want automatic upgrades but explicit/manual ones.

@GauthamGoli

@GauthamGoli
Copy link
Contributor Author

@sidmitra Agree with tagging releases, should we tag existing code in main as semantic version 1.0.0 ?

@sidmitra
Copy link
Contributor

@GauthamGoli

@sidmitra Agree with tagging releases, should we tag existing code in main as semantic version 1.0.0 ?

The current version is here:
https://github.com/Airbase/eventbusk/blob/main/pyproject.toml#L3
Feel free to update a minor version. 0.2.0 since it's mostly backward compatible. We can follow semver.

# TODO: Fix following
# Too many arguments for "Event" [call-arg]
event = event_type(**event_data) # type: ignore
setattr(event, 'event_id', event_id)
Copy link
Contributor Author

@GauthamGoli GauthamGoli Apr 28, 2022

Choose a reason for hiding this comment

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

Had to use setattr here instead because event_id field has default_factory=uuid.uuid4, init=False
and to keep things backwards compatible event_id will be set as None for older events

@GauthamGoli GauthamGoli merged commit 53919e2 into main May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants