You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "andrasbeni (via GitHub)" <gi...@apache.org> on 2023/02/13 15:54:21 UTC
[GitHub] [pulsar] andrasbeni opened a new pull request, #19503: [improve][meta]Allow version to start positive and grow by more than one
andrasbeni opened a new pull request, #19503:
URL: https://github.com/apache/pulsar/pull/19503
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. For details, see *[Guideline - Pulsar PR Naming Convention](https://docs.google.com/document/d/1d8Pw6ZbWk-_pCKdOmdvx9rnhPiyuxwq60_TrD68d7BA/edit#heading=h.trs9rsex3xom)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
-->
### Motivation
All currently available MetadataStore implementations assign versions to key-value pairs the same way: when a new pair is created, it is assigned version 0, and each subsequent modification increases the version by one. Actually, Pulsar does not need such a strict versioning scheme; it is enough that
* all versions are positive
* later versions are greater
* and the metadata store can inform us if a given Stat is represents the first version.
### Modifications
Introduced Stat.isFirstVersion that practically defaults to version==0
Relaxed the requirements imposed by the tests to only test what is actually necessary for Pulsar.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change is already covered by existing tests, such as the test modified by this PR.
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*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
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: https://github.com/andrasbeni/pulsar/pull/12
<!--
After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.
apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.
The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on pull request #19503: [improve][meta]Allow version to start positive and grow by more than one
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on PR #19503:
URL: https://github.com/apache/pulsar/pull/19503#issuecomment-1439279206
@eolivelli @lhotari
The change is to remove a restriction that is not anyway relevant to the usage that Pulsar is doing of metadata.
We're not relying anywhere on the fact that `version=0` means a record was just created. This restriction gets in the way of supporting other metadata backends where the versioning is different.
We could also argue that this change is made necessary by a not ideal API in MetadataStore (modeled on the way we used ZK API).
* After `store.put()`, we are checking if the record was modified or newly created.
* If it was created, we trigger an "artificial" notification event to invalidate the cache, such that it's kept consistent
I don't think it's strictly necessary to do so, though it's probably a good idea to maintain that.
Regarding the `isFirstVersion`, I believe it's the least invasive way of breaking the forced association "version=0 --> created".
Another way could be to add a `modifiedCount` in `Stat` that would contain the number of times a record has been modified after its creation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] andrasbeni commented on pull request #19503: [improve][meta]Allow version to start positive and grow by more than one
Posted by "andrasbeni (via GitHub)" <gi...@apache.org>.
andrasbeni commented on PR #19503:
URL: https://github.com/apache/pulsar/pull/19503#issuecomment-1431494112
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19503: [improve][meta]Allow version to start positive and grow by more than one
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19503:
URL: https://github.com/apache/pulsar/pull/19503#discussion_r1119338547
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/Stat.java:
##########
@@ -55,4 +62,9 @@ public class Stat {
* Whether the key-value pair had been created within the current "session".
*/
final boolean createdBySelf;
+
+ /**
+ * Whether this is the first version of the key-value pair since it has been last created.
+ */
+ final boolean firstVersion;
Review Comment:
Thanks for the example, that helps me to understand the name, and I agree that `created` wouldn't make sense in that context.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #19503: [improve][meta]Allow version to start positive and grow by more than one
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #19503:
URL: https://github.com/apache/pulsar/pull/19503#issuecomment-1429472601
It's possible that I have also misunderstood this change. Please share more context and reasoning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] andrasbeni commented on pull request #19503: [improve][meta]Allow version to start positive and grow by more than one
Posted by "andrasbeni (via GitHub)" <gi...@apache.org>.
andrasbeni commented on PR #19503:
URL: https://github.com/apache/pulsar/pull/19503#issuecomment-1431492852
@lhotari, @eolivelli, the goal of this change is to allow a broader range of third-party MetadataStore implementations to back Pulsar.
As I can see, the MetadataStore abstraction evolved from how Pulsar used ZooKeeper. Other implementations (Memory, RocksDB) were done with this API in mind and without constraints from an underlying store. In the case of Etcd, it was possible to adjust the behavior to be compatible with ZK by subtracting 1 from the version. But I see this as a compromise because the version of a KeyValue is not unique. (The same version can happen twice for the same key if it was deleted and recreated in the meantime.) The mod_revision field would have been a safer choice to implement compare-and-swap operations, but it did not follow ZK’s versioning scheme.
This change allows Pulsar to later switch to mod_revision in the case of Etcd and use stores where compare-and-swap is only allowed using fields similar to it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #19503: [improve][meta]Allow version to start positive and grow by more than one
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #19503:
URL: https://github.com/apache/pulsar/pull/19503#issuecomment-1429470368
> Actually, Pulsar does not need such a strict versioning scheme; it is enough that
>
> * all versions are positive
> * later versions are greater
> * and the metadata store can inform us if a given Stat is represents the first version.
Please provide reasoning for this. I believe that there's a reason for the existing strict versioning scheme. I'd suggest creating a PIP so that this gets discussed on the mailing list. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #19503: [improve][meta]Allow version to start positive and grow by more than one
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat merged PR #19503:
URL: https://github.com/apache/pulsar/pull/19503
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] andrasbeni commented on pull request #19503: [improve][meta]Allow version to start positive and grow by more than one
Posted by "andrasbeni (via GitHub)" <gi...@apache.org>.
andrasbeni commented on PR #19503:
URL: https://github.com/apache/pulsar/pull/19503#issuecomment-1439535653
/pulsarbot run-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #19503: [improve][meta]Allow version to start positive and grow by more than one
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19503:
URL: https://github.com/apache/pulsar/pull/19503#discussion_r1119256068
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/Stat.java:
##########
@@ -55,4 +62,9 @@ public class Stat {
* Whether the key-value pair had been created within the current "session".
*/
final boolean createdBySelf;
+
+ /**
+ * Whether this is the first version of the key-value pair since it has been last created.
+ */
+ final boolean firstVersion;
Review Comment:
Nit: does this always imply created? If so, why not use `boolean created` instead of tying the meaning to the concept of `version`? Based on the usage of this field in the PR, we want to know if this was a creation or a modification event, not if it was the `firstVersion`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a diff in pull request #19503: [improve][meta]Allow version to start positive and grow by more than one
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #19503:
URL: https://github.com/apache/pulsar/pull/19503#discussion_r1119332424
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/Stat.java:
##########
@@ -55,4 +62,9 @@ public class Stat {
* Whether the key-value pair had been created within the current "session".
*/
final boolean createdBySelf;
+
+ /**
+ * Whether this is the first version of the key-value pair since it has been last created.
+ */
+ final boolean firstVersion;
Review Comment:
`created` has the problem of being misleading on the get operation though.
If I do a `get()` and receive a value with `created` what does that really mean? The value is created in the sense that it exists :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org