You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "smiklosovic (via GitHub)" <gi...@apache.org> on 2024/03/16 22:45:15 UTC
[PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
smiklosovic opened a new pull request, #3180:
URL: https://github.com/apache/cassandra/pull/3180
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "iamaleksey (via GitHub)" <gi...@apache.org>.
iamaleksey commented on PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#issuecomment-2007522437
I would just cache the size value in a transient field in `HintsDescriptor`. A CHM is an overkill here.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1531938518
##########
src/java/org/apache/cassandra/hints/HintsWriter.java:
##########
@@ -259,7 +261,7 @@ void append(Hint hint) throws IOException
if (hintBuffer == buffer)
bytesWritten += totalSize;
else
- append((ByteBuffer) hintBuffer.flip());
Review Comment:
not necessary
##########
src/java/org/apache/cassandra/hints/HintsWriter.java:
##########
@@ -224,7 +226,7 @@ void append(ByteBuffer hint) throws IOException
* Serializes and appends the hint (with CRC included) to this session's aggregation buffer,
* writes to the underlying channel when the buffer is overflown.
*
- * Used mainly by tests and {@link LegacyHintsMigrator}
Review Comment:
does not exist anymore
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "iamaleksey (via GitHub)" <gi...@apache.org>.
iamaleksey commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1530617276
##########
src/java/org/apache/cassandra/hints/HintsStore.java:
##########
@@ -203,14 +213,21 @@ private void deleteHints(Predicate<HintsDescriptor> predicate)
{
dispatchDequeue.removeAll(removeSet);
corruptedFiles.removeAll(removeSet);
+ // if delete(descriptor) throws, hence
+ for (HintsDescriptor descriptor : removeSet)
+ hintsSizes.remove(descriptor.fileName());
}
}
void delete(HintsDescriptor descriptor)
{
File hintsFile = descriptor.file(hintsDirectory);
if (hintsFile.tryDelete())
+ {
+ // this method is also called standalone (without deleteHints)
Review Comment:
You are calling this twice on the normal path. Just leave the one call in this method, in `finally` if need be.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic closed pull request #3180: CASSANDRA-19477 trunk dont go to disk for hints sizes
URL: https://github.com/apache/cassandra/pull/3180
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "iamaleksey (via GitHub)" <gi...@apache.org>.
iamaleksey commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1530609416
##########
src/java/org/apache/cassandra/hints/HintsWriteExecutor.java:
##########
@@ -260,19 +261,33 @@ private void flushInternal(Iterator<ByteBuffer> iterator, HintsStore store)
long maxHintsFileSize = DatabaseDescriptor.getMaxHintsFileSize();
HintsWriter writer = store.getOrOpenWriter();
+ HintsDescriptor descriptor = writer.descriptor();
+ // If the creation of a new session throws, we have not written anything anyway,
+ // so we do not need to update hints sizes.
+ // If we throw during appending, we will track how many bytes were written in finally block
try (HintsWriter.Session session = writer.newSession(writeBuffer))
{
- while (iterator.hasNext())
+ try
+ {
+ while (iterator.hasNext())
+ {
+ session.append(iterator.next());
+ if (session.position() >= maxHintsFileSize)
+ break;
+ }
+ }
+ finally
{
- session.append(iterator.next());
- if (session.position() >= maxHintsFileSize)
- break;
+ Map<String, Long> hintsSizes = store.getHintsSizes();
+ hintsSizes.compute(descriptor.hintsFileName, (d, currentSize) ->
Review Comment:
But really just put is sufficient.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1531985023
##########
src/java/org/apache/cassandra/hints/HintsDescriptor.java:
##########
@@ -81,18 +81,26 @@ final class HintsDescriptor
final UUID hostId;
final int version;
final long timestamp;
+ final String hintsFileName;
+ final String crc32FileName;
final ImmutableMap<String, Object> parameters;
final ParameterizedClass compressionConfig;
private final Cipher cipher;
private final ICompressor compressor;
+ // size of underlying hint file (without descriptor header and crc file sizes)
+ // the value is populated in Session's close method in HintsWriter
+ volatile long size = 0;
Review Comment:
why not
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1530649079
##########
src/java/org/apache/cassandra/hints/HintsStore.java:
##########
@@ -61,6 +61,7 @@ final class HintsStore
private final Deque<HintsDescriptor> dispatchDequeue;
private final Queue<HintsDescriptor> corruptedFiles;
private final Map<HintsDescriptor, Long> hintsExpirations;
+ private final Map<String, Long> hintsSizes;
Review Comment:
It was like that before, then I changed it to String. Is not this just better? I don't mind to revert 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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1531943754
##########
src/java/org/apache/cassandra/hints/HintsWriter.java:
##########
@@ -271,6 +273,7 @@ public void close() throws IOException
flushBuffer();
maybeFsync();
maybeSkipCache();
+ descriptor.size = position();
Review Comment:
this should be probably in finally though, not sure ... as all above can throw
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "iamaleksey (via GitHub)" <gi...@apache.org>.
iamaleksey commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1530592560
##########
src/java/org/apache/cassandra/hints/HintsWriteExecutor.java:
##########
@@ -260,19 +261,33 @@ private void flushInternal(Iterator<ByteBuffer> iterator, HintsStore store)
long maxHintsFileSize = DatabaseDescriptor.getMaxHintsFileSize();
HintsWriter writer = store.getOrOpenWriter();
+ HintsDescriptor descriptor = writer.descriptor();
+ // If the creation of a new session throws, we have not written anything anyway,
+ // so we do not need to update hints sizes.
+ // If we throw during appending, we will track how many bytes were written in finally block
try (HintsWriter.Session session = writer.newSession(writeBuffer))
{
- while (iterator.hasNext())
+ try
+ {
+ while (iterator.hasNext())
+ {
+ session.append(iterator.next());
+ if (session.position() >= maxHintsFileSize)
+ break;
+ }
+ }
+ finally
{
- session.append(iterator.next());
- if (session.position() >= maxHintsFileSize)
- break;
+ Map<String, Long> hintsSizes = store.getHintsSizes();
+ hintsSizes.compute(descriptor.hintsFileName, (d, currentSize) ->
Review Comment:
You have `session.position()` ~~and `computeIfAbsent()`~~.
##########
src/java/org/apache/cassandra/hints/HintsWriteExecutor.java:
##########
@@ -260,19 +261,33 @@ private void flushInternal(Iterator<ByteBuffer> iterator, HintsStore store)
long maxHintsFileSize = DatabaseDescriptor.getMaxHintsFileSize();
HintsWriter writer = store.getOrOpenWriter();
+ HintsDescriptor descriptor = writer.descriptor();
+ // If the creation of a new session throws, we have not written anything anyway,
+ // so we do not need to update hints sizes.
+ // If we throw during appending, we will track how many bytes were written in finally block
try (HintsWriter.Session session = writer.newSession(writeBuffer))
{
- while (iterator.hasNext())
+ try
+ {
+ while (iterator.hasNext())
+ {
+ session.append(iterator.next());
+ if (session.position() >= maxHintsFileSize)
+ break;
+ }
+ }
+ finally
{
- session.append(iterator.next());
- if (session.position() >= maxHintsFileSize)
- break;
+ Map<String, Long> hintsSizes = store.getHintsSizes();
+ hintsSizes.compute(descriptor.hintsFileName, (d, currentSize) ->
Review Comment:
But really just `put()` is sufficient.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1531939360
##########
src/java/org/apache/cassandra/hints/HintsWriter.java:
##########
@@ -271,6 +273,7 @@ public void close() throws IOException
flushBuffer();
maybeFsync();
maybeSkipCache();
+ descriptor.size = position();
Review Comment:
why not to just update it in close() ? Then when this is used in try-with-resources in HintsWriteExecutor, we do not need to change there anything at all.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "iamaleksey (via GitHub)" <gi...@apache.org>.
iamaleksey commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1530627278
##########
src/java/org/apache/cassandra/hints/HintsWriteExecutor.java:
##########
@@ -260,19 +261,33 @@ private void flushInternal(Iterator<ByteBuffer> iterator, HintsStore store)
long maxHintsFileSize = DatabaseDescriptor.getMaxHintsFileSize();
HintsWriter writer = store.getOrOpenWriter();
+ HintsDescriptor descriptor = writer.descriptor();
+ // If the creation of a new session throws, we have not written anything anyway,
+ // so we do not need to update hints sizes.
+ // If we throw during appending, we will track how many bytes were written in finally block
try (HintsWriter.Session session = writer.newSession(writeBuffer))
{
- while (iterator.hasNext())
+ try
+ {
+ while (iterator.hasNext())
+ {
+ session.append(iterator.next());
+ if (session.position() >= maxHintsFileSize)
+ break;
+ }
+ }
+ finally
{
- session.append(iterator.next());
- if (session.position() >= maxHintsFileSize)
- break;
+ Map<String, Long> hintsSizes = store.getHintsSizes();
+ hintsSizes.compute(descriptor.hintsFileName, (d, currentSize) ->
Review Comment:
(This is a capturing lambda fyi)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1530671208
##########
src/java/org/apache/cassandra/hints/HintsStore.java:
##########
@@ -282,7 +297,11 @@ boolean isWriting()
HintsWriter getOrOpenWriter()
{
if (hintsWriter == null)
+ {
hintsWriter = openWriter();
+ hintsSizes.put(hintsWriter.descriptor().fileName(), hintsWriter.descriptor().file(hintsDirectory).length());
Review Comment:
we do not include .crc32 files too btw ... I can remove this though. There is a test at the bottom which thinks that by mere writing of a descriptor it should has some size. I can rewrite that test accordingly.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "iamaleksey (via GitHub)" <gi...@apache.org>.
iamaleksey commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1530698611
##########
src/java/org/apache/cassandra/hints/HintsStore.java:
##########
@@ -282,7 +293,11 @@ boolean isWriting()
HintsWriter getOrOpenWriter()
{
if (hintsWriter == null)
+ {
hintsWriter = openWriter();
+ hintsSizes.put(hintsWriter.descriptor(), hintsWriter.descriptor().file(hintsDirectory).length());
Review Comment:
I don't think we need to do 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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "iamaleksey (via GitHub)" <gi...@apache.org>.
iamaleksey commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1531951999
##########
src/java/org/apache/cassandra/hints/HintsWriter.java:
##########
@@ -259,7 +261,7 @@ void append(Hint hint) throws IOException
if (hintBuffer == buffer)
bytesWritten += totalSize;
else
- append((ByteBuffer) hintBuffer.flip());
Review Comment:
Not anymore in Java 11, was needed in Java 8 :)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "iamaleksey (via GitHub)" <gi...@apache.org>.
iamaleksey commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1530592560
##########
src/java/org/apache/cassandra/hints/HintsWriteExecutor.java:
##########
@@ -260,19 +261,33 @@ private void flushInternal(Iterator<ByteBuffer> iterator, HintsStore store)
long maxHintsFileSize = DatabaseDescriptor.getMaxHintsFileSize();
HintsWriter writer = store.getOrOpenWriter();
+ HintsDescriptor descriptor = writer.descriptor();
+ // If the creation of a new session throws, we have not written anything anyway,
+ // so we do not need to update hints sizes.
+ // If we throw during appending, we will track how many bytes were written in finally block
try (HintsWriter.Session session = writer.newSession(writeBuffer))
{
- while (iterator.hasNext())
+ try
+ {
+ while (iterator.hasNext())
+ {
+ session.append(iterator.next());
+ if (session.position() >= maxHintsFileSize)
+ break;
+ }
+ }
+ finally
{
- session.append(iterator.next());
- if (session.position() >= maxHintsFileSize)
- break;
+ Map<String, Long> hintsSizes = store.getHintsSizes();
+ hintsSizes.compute(descriptor.hintsFileName, (d, currentSize) ->
Review Comment:
You have `session.position()` and `computeIfAbsent()`.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1531939360
##########
src/java/org/apache/cassandra/hints/HintsWriter.java:
##########
@@ -271,6 +273,7 @@ public void close() throws IOException
flushBuffer();
maybeFsync();
maybeSkipCache();
+ descriptor.size = position();
Review Comment:
why not to just update it in close() ? Then when this is used in try-with-resources in HintsWriteExecutor, we do not need to change there anything at all and not mess with finally etc ...
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "iamaleksey (via GitHub)" <gi...@apache.org>.
iamaleksey commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1530702307
##########
src/java/org/apache/cassandra/hints/HintsWriteExecutor.java:
##########
@@ -260,19 +261,30 @@ private void flushInternal(Iterator<ByteBuffer> iterator, HintsStore store)
long maxHintsFileSize = DatabaseDescriptor.getMaxHintsFileSize();
HintsWriter writer = store.getOrOpenWriter();
+ HintsDescriptor descriptor = writer.descriptor();
+ // If the creation of a new session throws, we have not written anything anyway,
+ // so we do not need to update hints sizes.
+ // If we throw during appending, we will track how many bytes were written in finally block
try (HintsWriter.Session session = writer.newSession(writeBuffer))
{
- while (iterator.hasNext())
+ try
+ {
+ while (iterator.hasNext())
+ {
+ session.append(iterator.next());
+ if (session.position() >= maxHintsFileSize)
+ break;
+ }
+ }
+ finally
{
- session.append(iterator.next());
- if (session.position() >= maxHintsFileSize)
- break;
+ store.getHintsSizes().put(descriptor, session.position());
Review Comment:
No need to put this in a finally block I'd say.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "iamaleksey (via GitHub)" <gi...@apache.org>.
iamaleksey commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1531955613
##########
src/java/org/apache/cassandra/hints/HintsDescriptor.java:
##########
@@ -81,18 +81,26 @@ final class HintsDescriptor
final UUID hostId;
final int version;
final long timestamp;
+ final String hintsFileName;
+ final String crc32FileName;
final ImmutableMap<String, Object> parameters;
final ParameterizedClass compressionConfig;
private final Cipher cipher;
private final ICompressor compressor;
+ // size of underlying hint file (without descriptor header and crc file sizes)
+ // the value is populated in Session's close method in HintsWriter
+ volatile long size = 0;
Review Comment:
`size` -> `fileSize` slightly more descriptive. May also mark it as `transient`, to borrow the Accord convention (for the reader, not for the JVM).
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "clohfink (via GitHub)" <gi...@apache.org>.
clohfink commented on PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#issuecomment-2005166997
+1 thanks for 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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1530544132
##########
src/java/org/apache/cassandra/hints/HintsStore.java:
##########
@@ -76,9 +77,13 @@ private HintsStore(UUID hostId, File hintsDirectory, ImmutableMap<String, Object
dispatchDequeue = new ConcurrentLinkedDeque<>(descriptors);
corruptedFiles = new ConcurrentLinkedQueue<>();
hintsExpirations = new ConcurrentHashMap<>();
+ hintsSizes = new ConcurrentHashMap<>();
//noinspection resource
lastUsedTimestamp = descriptors.stream().mapToLong(d -> d.timestamp).max().orElse(0L);
+
+ for (HintsDescriptor descriptor : Iterables.concat(dispatchDequeue, corruptedFiles))
Review Comment:
it is not technically necessary to iterate over corrputedFiles because they are empty here
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "iamaleksey (via GitHub)" <gi...@apache.org>.
iamaleksey commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1530546676
##########
src/java/org/apache/cassandra/hints/HintsStore.java:
##########
@@ -61,6 +61,7 @@ final class HintsStore
private final Deque<HintsDescriptor> dispatchDequeue;
private final Queue<HintsDescriptor> corruptedFiles;
private final Map<HintsDescriptor, Long> hintsExpirations;
+ private final Map<String, Long> hintsSizes;
Review Comment:
Why not `HintsDescriptor` as key?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "iamaleksey (via GitHub)" <gi...@apache.org>.
iamaleksey commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1530593679
##########
src/java/org/apache/cassandra/hints/HintsStore.java:
##########
@@ -282,7 +297,11 @@ boolean isWriting()
HintsWriter getOrOpenWriter()
{
if (hintsWriter == null)
+ {
hintsWriter = openWriter();
+ hintsSizes.put(hintsWriter.descriptor().fileName(), hintsWriter.descriptor().file(hintsDirectory).length());
Review Comment:
This seems pretty unnecessary or at the very least unjustified.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-19477 trunk dont go to disk for hints sizes [cassandra]
Posted by "iamaleksey (via GitHub)" <gi...@apache.org>.
iamaleksey commented on code in PR #3180:
URL: https://github.com/apache/cassandra/pull/3180#discussion_r1530675507
##########
src/java/org/apache/cassandra/hints/HintsStore.java:
##########
@@ -282,7 +297,11 @@ boolean isWriting()
HintsWriter getOrOpenWriter()
{
if (hintsWriter == null)
+ {
hintsWriter = openWriter();
+ hintsSizes.put(hintsWriter.descriptor().fileName(), hintsWriter.descriptor().file(hintsDirectory).length());
Review Comment:
CRC files don't matter, an entirely different order of magnitude where total limits are involved.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org