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 TableView support #743

Merged
merged 3 commits into from
Mar 31, 2022
Merged

Add TableView support #743

merged 3 commits into from
Mar 31, 2022

Conversation

ZiyaoWei
Copy link
Contributor

@ZiyaoWei ZiyaoWei commented Mar 9, 2022

Fixes apache/pulsar#13929

Motivation

This adds TableView support to the Golang client so it can have feature parity with the Java client.

See apache/pulsar#12356 and apache/pulsar#12838 for context.

Modifications

Add TableView and Client.CreateTableView and so users can track a keyed topic as a map.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • go test -timeout 5s -run ^TestTableView$ github.com/apache/pulsar-client-go/pulsar

Does this pull request potentially affect one of the following parts:

  • The public API: yes

Documentation

  • Does this pull request introduce a new feature? yes
  • If yes, how is the feature documented? GoDocs

@ZiyaoWei
Copy link
Contributor Author

ZiyaoWei commented Mar 9, 2022

A few things worth noting:

  • This is stacked on top of Add TableView support #743;
  • The Java implementation actually return views of the underlying map in, e.g., Keys(), which I think might be problematic since then the users could inadvertently change the entries in the TableView. Maybe we should change it to returning a copy of the underlying values (or maybe I am missing something)?
  • The internal table is a map which aren't thread safe - I'll try changing it to sync.Map which should make the code less verbose, although since Golang doesn't have generics internally we have to do some type assertions. Alternatively, if we are okay with changing the accessors to return copies we can do the same and stick with a map.
  • Since (again) Golang doesn't support generics yet there's no good way of knowing what the types of the values would be, so I am making the API similar to GetSchemaValue(v interface{}) by letting the users pass the reflect.Type of the values. This feels a bit ugly but I can't think of a better alternative, suggestions are most welcome!

Schema Schema

// SchemaValueType represents the type of values for the given schema.
SchemaValueType reflect.Type
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious how does is the Schema related to the SchemaValueType?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for taking so long! Basically the types have to match, but since Golang has no generics yet, there's no good way of getting the types out of the Schema, and while for things like strings and ints the type can be inferred, in cases like protos there's no way to do that. I guess we can add the field in the Schema itself but that feels like a larger change and isn't useful for most users of schemas?

pulsar/table_view.go Show resolved Hide resolved
pulsar/table_view_impl.go Outdated Show resolved Hide resolved
pulsar/table_view_impl.go Outdated Show resolved Hide resolved
pulsar/table_view_impl.go Outdated Show resolved Hide resolved
@wolfstudy wolfstudy added this to the v0.9.0 milestone Mar 22, 2022
@ZiyaoWei
Copy link
Contributor Author

Sorry for the slow turnaround - I somehow messed up my local dev environment. I've discussed with @nlu90 and filed apache/pulsar#14821, so I'll change the corresponding methods to returning copies of the underlying data.

@shoothzj
Copy link
Member

@merlimat @ZiyaoWei can we have a method for TableView to let user know the consume progress? Like, pulsar function's start, it will check if the latestMsgId is consumed

@merlimat
Copy link
Contributor

@merlimat @ZiyaoWei can we have a method for TableView to let user know the consume progress? Like, pulsar function's start, it will check if the latestMsgId is consumed

@shoothzj I'm sure I follow your suggestion. Can you make a concrete example? Also, this is just porting the TableView feature already present in Java client to Go, with the same exact semantics. I think we should have a new PIP to add more features TableView

@shoothzj
Copy link
Member

shoothzj commented Mar 31, 2022

// sync make sure the tableView's consume progress beyond this time
func (tv *TableViewImpl) sync(time time.Time) {

}

// syncLatest make sure the tableView's consume progress beyond the latestMsgId when this function are called
func (tv *TableViewImpl) syncLatest() {

}

@merlimat Thanks, I know the background. I was wondering if we can have methods like the below. If that make sense, I can write a PIP.

@merlimat
Copy link
Contributor

@shoothzj The question I have on these methods is what would be their usage. Eg: even if you do syncLatest(), by the time you're using the map it could already be stale with new messages being published.

In the meantime, I'm going ahead to merge this as it includes the features described in PIP-104.

@merlimat merlimat merged commit 3e1b39d into apache:master Mar 31, 2022
@ZiyaoWei ZiyaoWei deleted the tableView branch March 31, 2022 17:47
@shoothzj
Copy link
Member

shoothzj commented Apr 1, 2022

@merlimat let's continue our discussion in slack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Golang Client] New consumer type: TableView
5 participants