Re: [PR] KAFKA-14412: Generalise over RocksDB WriteBatch [kafka]
ableegoldman merged PR #14853: URL: https://github.com/apache/kafka/pull/14853 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14412: Generalise over RocksDB WriteBatch [kafka]
ableegoldman commented on PR #14853: URL: https://github.com/apache/kafka/pull/14853#issuecomment-1863466888 I honestly don't know...maybe try doing a rebase from trunk and force pushing? That can often clear up this sort of thing. Ideally we don't have to abandon this PR for a fresh one (just to avoid losing history/review/comments), but that can be a last resort I suppose -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14412: Generalise over RocksDB WriteBatch [kafka]
nicktelford commented on PR #14853: URL: https://github.com/apache/kafka/pull/14853#issuecomment-1858152008 The build still appears to be failing, this time with an error from git about updating the HEAD... It doesnt look like other PRs are affected by this, so I'm guessing the build is cached per-PR and something has borked the cached build for this PR. Shall I just close this PR and open a new one from the same branch? Might give it a fresh build that can then succeed. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14412: Generalise over RocksDB WriteBatch [kafka]
ableegoldman commented on PR #14853: URL: https://github.com/apache/kafka/pull/14853#issuecomment-1856573877 You need to log in to restart the build, so it might be that only committers can trigger a retry...which is annoying (though I guess I kind of understand, don't want some random person to waste money & compute by spamming retries. We've had weirdos spam kafka before so it's not unthinkable 路♀️ ) Anyways I replayed it but looks like now there's some issue with Jenkins and/or git itself, so I replayed it again just now. FWIW I really don't think this PR is breaking anything, but I just can't merge with a completely broken build. Hopefully this time it goes through 爛  -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14412: Generalise over RocksDB WriteBatch [kafka]
nicktelford commented on PR #14853: URL: https://github.com/apache/kafka/pull/14853#issuecomment-1855684906 I couldn't figure out how to restart the build, but I've rebased against trunk in the hope that some of these failing tests have been addressed upstream. I'm confident they aren't introduced by this PR. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14412: Generalise over RocksDB WriteBatch [kafka]
ableegoldman commented on PR #14853: URL: https://github.com/apache/kafka/pull/14853#issuecomment-1854731072 Two of the builds failed again...one of them looks like a failing Connect test bailed out with a sys exit, not sure what happened with the other one, but let's retry 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14412: Generalise over RocksDB WriteBatch [kafka]
nicktelford commented on PR #14853: URL: https://github.com/apache/kafka/pull/14853#issuecomment-1844992547 There was a merge conflict with trunk that has now been resolved. @ableegoldman once the build passes, can we merge this? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14412: Generalise over RocksDB WriteBatch [kafka]
ableegoldman commented on PR #14853: URL: https://github.com/apache/kafka/pull/14853#issuecomment-1841596989 Strangely one of the builds failed to compile due to some issue in the upgrade test/smoke client -- seems unrelated but we can't merge with a broken build, so let's retry and hope it goes away 爛 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14412: Generalise over RocksDB WriteBatch [kafka]
nicktelford commented on PR #14853: URL: https://github.com/apache/kafka/pull/14853#issuecomment-1838948918 I've rebased against trunk, so it's ready to be merged -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14412: Generalise over RocksDB WriteBatch [kafka]
nicktelford commented on code in PR #14853:
URL: https://github.com/apache/kafka/pull/14853#discussion_r1413707064
##
streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBStore.java:
##
@@ -572,13 +574,19 @@ public synchronized void flush() {
@Override
public void addToBatch(final KeyValue record,
- final WriteBatch batch) throws RocksDBException {
+ final WriteBatchInterface batch) throws
RocksDBException {
dbAccessor.addToBatch(record.key, record.value, batch);
}
@Override
-public void write(final WriteBatch batch) throws RocksDBException {
-db.write(wOptions, batch);
+public void write(final WriteBatchInterface batch) throws RocksDBException
{
+if (batch instanceof WriteBatch) {
+db.write(wOptions, (WriteBatch) batch);
+} else if (batch instanceof WriteBatchWithIndex) {
+db.write(wOptions, (WriteBatchWithIndex) batch);
+} else {
+throw new ProcessorStateException("Unknown type of batch " +
batch.getClass().getCanonicalName());
Review Comment:
Good point! Done.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-14412: Generalise over RocksDB WriteBatch [kafka]
ableegoldman commented on code in PR #14853:
URL: https://github.com/apache/kafka/pull/14853#discussion_r1412620201
##
streams/src/main/java/org/apache/kafka/streams/state/internals/RocksDBStore.java:
##
@@ -572,13 +574,19 @@ public synchronized void flush() {
@Override
public void addToBatch(final KeyValue record,
- final WriteBatch batch) throws RocksDBException {
+ final WriteBatchInterface batch) throws
RocksDBException {
dbAccessor.addToBatch(record.key, record.value, batch);
}
@Override
-public void write(final WriteBatch batch) throws RocksDBException {
-db.write(wOptions, batch);
+public void write(final WriteBatchInterface batch) throws RocksDBException
{
+if (batch instanceof WriteBatch) {
+db.write(wOptions, (WriteBatch) batch);
+} else if (batch instanceof WriteBatchWithIndex) {
+db.write(wOptions, (WriteBatchWithIndex) batch);
+} else {
+throw new ProcessorStateException("Unknown type of batch " +
batch.getClass().getCanonicalName());
Review Comment:
nit: log an error before we throw the exception
Also, imo this should just be an IllegalStateException instead of a
ProcessorStateException
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-14412: Generalise over RocksDB WriteBatch [kafka]
nicktelford commented on PR #14853: URL: https://github.com/apache/kafka/pull/14853#issuecomment-1836027902 @mjsax @ableegoldman @lucasbru @wcarlson5 @bbejeck @vvcephei @guozhangwang -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-14412: Generalise over RocksDB WriteBatch [kafka]
nicktelford commented on PR #14853: URL: https://github.com/apache/kafka/pull/14853#issuecomment-1829908202 This is part of KIP-892, and has been broken out into a separate PR to reduce the review burden on the main KIP-892 implementation, since it can be merged independently. Note: KIP-892 makes use of `WriteBatchWithIndex` for its transaction buffer, so these changes are needed to enable us to use both `WriteBatch` (which continue to be used in `restoreBatch`) and `WriteBatchWithIndex` (which are used for transaction buffers). @cadonna -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] KAFKA-14412: Generalise over RocksDB WriteBatch [kafka]
nicktelford opened a new pull request, #14853: URL: https://github.com/apache/kafka/pull/14853 The type hierarchy of RocksDB's `WriteBatch` looks like this: ``` +-+ | WriteBatchInterface | +-+ ^ | +-+ | AbstractWriteBatch | +-+ ^ | +--+--+ | | +++-+ | WriteBatch || WriteBatchWithIndex | +++-+ ``` By switching our `BatchWritingStore` methods from `WriteBatch` to `WriteBatchInterface`, we enable the use of `WriteBatchWithIndex` as well. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
