[improve][client] PIP-302 Introduce refreshAsync API for TableView

[liangyepianzhou]

Motivation

Prerequisite: Since messages are constantly being written into the Topic and there is no read-write lock guarantee, we cannot assure the retrieval of the most up-to-date value.
Implementation Goal: Record a checkpoint before reading and ensure the retrieval of the latest value of the key up to this checkpoint.
Use Case: When read and write operations for a certain key do not occur simultaneously, we can refresh the TableView before reading the key to obtain the latest value for this key.

Modification

Introduce a new API refreshAsync.

Verifying this change

• Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

• Added integration tests for end-to-end deployment with large payloads (10MB)
• Extended integration test for recovery after broker failure

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

If the box was checked, please highlight the changes

• Dependencies (add or upgrade a dependency)
• The public API
• The schema
• The default values of configurations
• The threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• The metrics
• Anything that affects deployment

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository:

[mattisonchao]

I just suddenly realised there's a bug here. In the worst condition, the publisher publishes messages continuously, and the reader.hasMessageAvailableAsync() always returns true. The tableview will stuck for a long time. :/

We should give a checkpoint before starting the infinity loop.

[RobertIndie]

+1

[liangyepianzhou]

Good suggestion. This is a latent issue that has existed previously. I was initially vacillating on whether to incorporate the fix within this proposal. I am grateful for your input; let us then incorporate this resolution into this proposal.

[liangyepianzhou]

Alternatively, it might be more beneficial to create a separate PR solely for the purpose of rectifying this bug.

[liangyepianzhou]

I raise a PR to fix it. Feel free to give comments.

[codelipenghui]

It's not good to expose this method directly to the TableView interface because the method named read something but nothing returned to the caller.

I think maybe refresh() is better.

table.refresh().whenComplete((__, ex) -> {
       if (ex == null) {
              table.get(key);
       }
})

Add we should also provide a clear description of the behavior for the new method.

[codelipenghui]

And for this PR.

It's not an issue of the existing TableView implementation, but it's an issue if you want to expose readAllExistingMessages()method to users.

[liangyepianzhou]

Why isn't this an issue with the existing implementation? If there are a large number of producers sending messages to a single topic, then the delay in building the TableView for this topic could be very large, and in extreme cases, it may never be completed.

[liangyepianzhou]

I think maybe refresh() is better.

table.refresh().whenComplete((__, ex) -> {
       if (ex == null) {
              table.get(key);
       }
})

Add we should also provide a clear description of the behavior for the new method.

Great suggestion; I've updated the notes.

[merlimat]

If we want to allow to implement different consistency models, it would be better to have that as part of the TableView configuration builder, instead of requiring a user to read everything that was published.

[liangyepianzhou]

If we want to allow to implement different consistency models, it would be better to have that as part of the TableView configuration builder, instead of requiring a user to read everything that was published.

Good suggestion. I have made the change as you suggested.

[liangyepianzhou]

@merlimat Following our discussion, there might be some potential downsides to modifying the tableViewBuilder as follows:

1. We would need to add many asynchronous interfaces, such as T getAsync(String key); boolean containsKeyAsync(String key); Collection valuesAsync(); etc.
2. Even if we refresh the tableView before each data read, we can't guarantee that the data retrieved is the most up-to-date. This configuration might confuse users.

[liangyepianzhou]

Prerequisite: Since messages are constantly being written into the Topic and there is no read-write lock guarantee, we cannot assure the retrieval of the most up-to-date value.
Implementation Goal: Record a checkpoint before reading and ensure the retrieval of the latest value of the key up to this checkpoint.
Use Case: When read and write operations for a certain key do not occur simultaneously, we can refresh the TableView before reading the key to obtain the latest value for this key.

Based on the above, I believe it would be more reasonable to introduce an asynchronous API refreshAsync(). There are two reasons for this:

• It provides more flexibility for the user.
• The operation of refresh is easier for users to understand.

[liangyepianzhou]

@merlimat @codelipenghui, I changed the design from adding alwaysRefresh configuration option for TableViewBuilder to Add refreshAsync method. If necessary, we can add the alwaysRefresh configuration option for TableViewBuilder in the future.
Please help to leave your valuable feedback. Thanks.

[codelipenghui]

@merlimat @codelipenghui, I changed the design from adding alwaysRefresh configuration option for TableViewBuilder to Add refreshAsync method. If necessary, we can add the alwaysRefresh configuration option for TableViewBuilder in the future.
Please help to leave your valuable feedback. Thanks.

@liangyepianzhou +1, I think we can continue the proposal.