You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "vcrfxia (via GitHub)" <gi...@apache.org> on 2023/04/19 22:41:28 UTC
[GitHub] [kafka] vcrfxia opened a new pull request, #13615: KAFKA-14834: [12/N] Minor code cleanups relating to versioned stores
vcrfxia opened a new pull request, #13615:
URL: https://github.com/apache/kafka/pull/13615
(This PR is stacked on https://github.com/apache/kafka/pull/13609. Only commits starting from `whitespace cleanup` need to be reviewed separately.)
This PR contains various minor/non-functional refactors requested from previous PR reviews. The only change which is borderline functional is that the RocksDB versioned store implementation now throws `InvalidStateStoreException` if any of its public methods are accessed before the store is initialized/opened. (None of the methods would have worked before anyway, but now the error message is cleaner.)
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vcrfxia commented on a diff in pull request #13615: KAFKA-14834: [12/N] Minor code cleanups relating to versioned stores
Posted by "vcrfxia (via GitHub)" <gi...@apache.org>.
vcrfxia commented on code in PR #13615:
URL: https://github.com/apache/kafka/pull/13615#discussion_r1171934110
##########
streams/src/main/java/org/apache/kafka/streams/state/KeyValueStore.java:
##########
@@ -36,6 +37,7 @@
* @param value The value to update, it can be {@code null};
* if the serialized bytes are also {@code null} it is interpreted as deletes
* @throws NullPointerException If {@code null} is used for key.
+ * @throws InvalidStateStoreException if the store is not initialized
Review Comment:
Went through and added this additional javadoc line to the methods where it seemed to be missing. Looks like the usage throughout the codebase is very inconsistent though:
* `RocksDBStore` enforces it on all methods even though it wasn't documented in `KeyValueStore`. The in-memory implementations do not enforce it.
* `WindowStore` has docs about this but the actual implementations do not enforce it.
* `VersionedKeyValueStore` has docs but the implementation was not enforcing it. I've reconciled this inconsistency in this PR, but the others are larger changes that I'd like to leave for later.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on pull request #13615: KAFKA-14834: [12/N] Minor code cleanups relating to versioned stores
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on PR #13615:
URL: https://github.com/apache/kafka/pull/13615#issuecomment-1520833481
Merged to `trunk` and cherry-picked to `3.5`.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax merged pull request #13615: KAFKA-14834: [12/N] Minor code cleanups relating to versioned stores
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax merged PR #13615:
URL: https://github.com/apache/kafka/pull/13615
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vcrfxia commented on a diff in pull request #13615: KAFKA-14834: [12/N] Minor code cleanups relating to versioned stores
Posted by "vcrfxia (via GitHub)" <gi...@apache.org>.
vcrfxia commented on code in PR #13615:
URL: https://github.com/apache/kafka/pull/13615#discussion_r1171934110
##########
streams/src/main/java/org/apache/kafka/streams/state/KeyValueStore.java:
##########
@@ -36,6 +37,7 @@
* @param value The value to update, it can be {@code null};
* if the serialized bytes are also {@code null} it is interpreted as deletes
* @throws NullPointerException If {@code null} is used for key.
+ * @throws InvalidStateStoreException if the store is not initialized
Review Comment:
Went through and added this additional javadoc line to the methods where it seemed to be missing (as requested in https://github.com/apache/kafka/pull/13188#discussion_r1097993808). Looks like the usage throughout the codebase is very inconsistent though:
* `RocksDBStore` enforces it on all methods even though it wasn't documented in `KeyValueStore`. The in-memory implementations do not enforce it.
* `WindowStore` has docs about this but the actual implementations do not enforce it.
* `VersionedKeyValueStore` has docs but the implementation was not enforcing it. I've reconciled this inconsistency in this PR, but the others are larger changes that I'd like to leave for later.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vcrfxia commented on a diff in pull request #13615: KAFKA-14834: [12/N] Minor code cleanups relating to versioned stores
Posted by "vcrfxia (via GitHub)" <gi...@apache.org>.
vcrfxia commented on code in PR #13615:
URL: https://github.com/apache/kafka/pull/13615#discussion_r1171934110
##########
streams/src/main/java/org/apache/kafka/streams/state/KeyValueStore.java:
##########
@@ -36,6 +37,7 @@
* @param value The value to update, it can be {@code null};
* if the serialized bytes are also {@code null} it is interpreted as deletes
* @throws NullPointerException If {@code null} is used for key.
+ * @throws InvalidStateStoreException if the store is not initialized
Review Comment:
Went through and added this additional javadoc line to the methods where it seemed to be missing (as requested in https://github.com/apache/kafka/pull/13188#discussion_r1097993808). Looks like the usage throughout the codebase is very inconsistent though:
* `RocksDBStore` enforces it on all methods even though it wasn't documented in `KeyValueStore`. The in-memory implementations do not enforce it.
* `WindowStore` has docs about this but the actual implementations do not enforce it.
* `SessionStore` neither has the annotations nor implements it.
* `VersionedKeyValueStore` has docs but the implementation was not enforcing it. I've reconciled this inconsistency in this PR, but the others are larger changes that I'd like to leave for later.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org