[pip] PIP-428: Change TopicPoliciesService interface to fix consistency issues

[lhotari]

Motivation

Change TopicPoliciesService interface to fix consistency issues with Topic Policies. Example issues #24393, #21303

Modifications

This is a PIP document "PIP-428: Change TopicPoliciesService interface to fix consistency issues".
The WIP implementation PR is #24427 .

Documentation

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

[codelipenghui]

Thanks for driving the proposal. And the proposed solution looks good to me.

For the critical issues that affect production systems, if there is any link or code snippet to show the existing issues (How current implementation looks like), it will be great to help the reviewers to understand the issue quickly.

[lhotari]

Thanks for driving the proposal. And the proposed solution looks good to me.

For the critical issues that affect production systems, if there is any link or code snippet to show the existing issues (How current implementation looks like), it will be great to help the reviewers to understand the issue quickly.

@codelipenghui Thanks for the feedback. I'll follow up by adding more examples of what could go wrong when an intended update gets lost. #24393 contains examples of lost updates.

[lhotari]

Thanks for driving the proposal. And the proposed solution looks good to me.
For the critical issues that affect production systems, if there is any link or code snippet to show the existing issues (How current implementation looks like), it will be great to help the reviewers to understand the issue quickly.

@codelipenghui Thanks for the feedback. I'll follow up by adding more examples of what could go wrong when an intended update gets lost. #24393 contains examples of lost updates.

@codelipenghui I added a code example of lost updates with commit 32d9797 .

I'm not sure if more examples would be needed to explain why it's critical for production systems that topic policies work as expected and don't loose changes. If a topic gets configured with incorrect policy, it could lead to multiple issues. One possible issue would be that there's a namespace level setting for shorter retention and there's a requirement that a particular topic which should have a longer retention. Let's say that the user configures multiple different settings for the topic policy and the retention setting gets lost. When this retention setting for topic policy gets lost, data will get lost unexpectedly.

[codelipenghui]

I added a code example of lost updates with commit 32d9797 .

Thanks for providing the test case.

One possible issue would be that there's a namespace level setting for shorter retention and there's a requirement that a particular topic which should have a longer retention. Let's say that the user configures multiple different settings for the topic policy and the retention setting gets lost. When this retention setting for topic policy gets lost, data will get lost unexpectedly.

It should not happen since we have enabled topic compaction for system topics, which means infinite topic retention. And we also disabled message TTL, backlog quota on system topics as well.

I'm not sure if more examples would be needed to explain why it's critical for production systems that topic policies work as expected and don't loose changes.

I'm good with the new test case. I tried to debug and understand why it loses updates. The major reason is read-write consistency issue which you already mentioned in the proposal

[BewareMyPower]

Overall LGTM.

There is one point that it's not clear when should skipUpdateWhenTopicPolicyDoesntExist be true? It would be better to document when will a plugin's method be called in Pulsar, as well as what are the arguments. This will be good for plugin authors to understand the interface to implement.

[BewareMyPower]

BTW, unlike the interface designed to be called by users, the interface designed to be customized by users and only called internally should have a different document style.

Take SchemaStorage for example, there are two put methods:

pulsar/pulsar-common/src/main/java/org/apache/pulsar/common/protocol/schema/SchemaStorage.java

Lines 31 to 45 in 73a4ae4

	CompletableFuture<SchemaVersion> put(String key, byte[] value, byte[] hash);
	/**
	* Put the schema to the schema storage.
	*
	* @param key The schema ID
	* @param fn The function to calculate the value and hash that need to put to the schema storage
	* The input of the function is all the existing schemas that used to do the schemas compatibility check
	* @return The schema version of the stored schema
	*/
	default CompletableFuture<SchemaVersion> put(String key,
	Function<CompletableFuture<List<CompletableFuture<StoredSchema>>>,
	CompletableFuture<Pair<byte[], byte[]>>> fn) {
	return fn.apply(getAll(key)).thenCompose(pair -> put(key, pair.getLeft(), pair.getRight()));
	}

Actually the 1st put overload is never used out of the built-in implementation. The 2nd put overload is actually used. But it's very confusing for a downstream plugin's author. Actually I have encountered NPE before since only the 1st method is implemented.

[lhotari]

Overall LGTM.

There is one point that it's not clear when should skipUpdateWhenTopicPolicyDoesntExist be true? It would be better to document when will a plugin's method be called in Pulsar, as well as what are the arguments. This will be good for plugin authors to understand the interface to implement.

@BewareMyPower Thanks for the feedback. skipUpdateWhenTopicPolicyDoesntExist should be set to true only for updates that are removals from an existing policy and could be omitted when the policy doesn't already exist. I'll take a look if I can clarify this and add better documentation.

[lhotari]

@BewareMyPower I made some updates to address your feedback.

[lhotari]

I have started a VOTE thread at https://lists.apache.org/thread/ndh0dhfjqf1htcmodgyyqt6k4bgdfj3x , please vote.

[BewareMyPower]

It's clear now

[lhotari]

PIP vote passed: https://lists.apache.org/thread/ndh0dhfjqf1htcmodgyyqt6k4bgdfj3x