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 read-only status and event timestamps to metadata #276

Merged
merged 12 commits into from
Oct 27, 2020

Conversation

Jmgr
Copy link
Contributor

@Jmgr Jmgr commented Oct 13, 2020

This PR adds the read-only status to the metadata, as well as the first and latest timestamps of received messages, paused and read-only status changes.

I wasn't able to test this for now because FetchPartitionMetadata has not been pushed to go-liftbridge yet, so creating this PR as a draft.

@LaPetiteSouris
Copy link
Contributor

LaPetiteSouris commented Oct 13, 2020

This PR adds the read-only status to the metadata, as well as the first and latest timestamps of received messages, paused and read-only status changes.

I wasn't able to test this for now because FetchPartitionMetadata has not been pushed to go-liftbridge yet, so creating this PR as a draft.

My bad. FetchPartitionMetadata is implemented and I plan to open a PR to go-liftbridge today or tomorrow. I actually felt on some issues with testing on go-liftbridge so I spent time working on that first.

I think by tomorrow I will open a PR on go-liftbridge for FetchPartitionMetadata

server/partition.go Outdated Show resolved Hide resolved
server/partition.go Outdated Show resolved Hide resolved
server/partition.go Show resolved Hide resolved
server/protocol/internal.proto Outdated Show resolved Hide resolved
server/stream.go Outdated Show resolved Hide resolved
server/metadata.go Outdated Show resolved Hide resolved
Co-authored-by: Tyler Treat <ttreat31@gmail.com>
@LaPetiteSouris
Copy link
Contributor

FYI, you may need this to test the feature

server/partition.go Outdated Show resolved Hide resolved
@tylertreat
Copy link
Member

Changes lgtm. Would be good to get some tests around the changes to FetchPartitionMetadata.

@Jmgr
Copy link
Contributor Author

Jmgr commented Oct 21, 2020

Changes lgtm. Would be good to get some tests around the changes to FetchPartitionMetadata.

Yes. I am waiting for liftbridge-io/go-liftbridge#90 to be merged, so I can create a PR for go-liftbridge adding the timestamps. I would then use it to add some API tests (in api_test.go) using FetchPartitionMetadata. I though about adding metadata tests (in metadata_test.go), but didn't find a way to simulate message publication using the metadataAPI to trigger a timestamps update, but I may have missed something?

@LaPetiteSouris
Copy link
Contributor

LaPetiteSouris commented Oct 21, 2020

Changes lgtm. Would be good to get some tests around the changes to FetchPartitionMetadata.

Yes. I am waiting for liftbridge-io/go-liftbridge#90 to be merged, so I can create a PR for go-liftbridge adding the timestamps. I would then use it to add some API tests (in api_test.go) using FetchPartitionMetadata. I though about adding metadata tests (in metadata_test.go), but didn't find a way to simulate message publication using the metadataAPI to trigger a timestamps update, but I may have missed something?

I guess a small unit test around getPartitionMetadata may work. ( I may make a small suggestion above )

IMO, the new feature you will add in the client may be tested separately using api mock (already in the client).

@tylertreat
Copy link
Member

@Jmgr Right, I forgot the go-liftbridge PR hasn't been merged yet.

@Jmgr
Copy link
Contributor Author

Jmgr commented Oct 22, 2020

IMO, the new feature you will add in the client may be tested separately using api mock (already in the client).

I don't think so, unfortunately, because to test the received message timestamps you need to be able to publish a message. The changes in liftbridge-io/go-liftbridge#93 should allow testing this.

@Jmgr
Copy link
Contributor Author

Jmgr commented Oct 22, 2020

Changes lgtm. Would be good to get some tests around the changes to FetchPartitionMetadata.

Done.

@tylertreat
Copy link
Member

lgtm, I think it just needs dependencies to be updated?

@Jmgr
Copy link
Contributor Author

Jmgr commented Oct 23, 2020

lgtm, I think it just needs dependencies to be updated?

Yes, I'm waiting for liftbridge-io/go-liftbridge#93 to be merged. I will remove the replace instruction then.

@Jmgr Jmgr marked this pull request as ready for review October 23, 2020 09:11
@tylertreat
Copy link
Member

I've merged liftbridge-io/go-liftbridge#93

@Jmgr
Copy link
Contributor Author

Jmgr commented Oct 27, 2020

I've merged liftbridge-io/go-liftbridge#93

I have updated the dependency.

@tylertreat tylertreat merged commit 7c3dd14 into liftbridge-io:master Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants