-
Notifications
You must be signed in to change notification settings - Fork 14.5k
MINOR: Cleanups in storage module #20087
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR, left some comments
public final long recoveryPoint; | ||
public final LogOffsetMetadata nextOffsetMetadata; | ||
|
||
public record LoadedLogOffsets(long logStartOffset, long recoveryPoint, LogOffsetMetadata nextOffsetMetadata) { | ||
public LoadedLogOffsets(final long logStartOffset, | ||
final long recoveryPoint, | ||
final LogOffsetMetadata nextOffsetMetadata) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remain equals()
, hashCode()
, toString()
for this record?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed equals()
and hashCode()
|
||
int i = 0; | ||
for (RecordBatch batch : validatedRecords.batches()) { | ||
assertTrue(batch.isValid()); | ||
assertEquals(batch.timestampType(), TimestampType.CREATE_TIME); | ||
assertEquals(TimestampType.CREATE_TIME, batch.timestampType()); | ||
maybeCheckBaseTimestamp(timestampSeq.get(0), batch); | ||
assertEquals(batch.maxTimestamp(), batch.maxTimestamp()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion also can remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are the same issue at L365, L1811, We could update the actual value to TestUtils.toList(batch).stream().map(Record::timestamp).max(Long::compare).get()
, the root cause is #16167
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also found this issue while reviewing this PR and have already filed a MINOR to fix it before the comments. #20093
public final short producerEpoch; | ||
|
||
public LastRecord(OptionalLong lastDataOffset, short producerEpoch) { | ||
public record LastRecord(OptionalLong lastDataOffset, short producerEpoch) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove equals
and hashCode
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@mimaison I think you meant the 'storage' module in the PR title. |
@@ -34,6 +34,12 @@ | |||
import java.util.Collection; | |||
import java.util.Iterator; | |||
|
|||
/** | |||
* @param <R1> The type of records used to formulate the expectations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formatting of the type parameters in this javadoc comment is a little odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I blame IntelliJ :) Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mimaison thanks for this cleanup
public int quotaWindowSizeSeconds() { | ||
return quotaWindowSizeSeconds; | ||
} | ||
|
||
@Override | ||
public String toString() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the generated toString
should be good enough I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
@@ -94,7 +94,7 @@ public OptionalLong read() { | |||
Content content = Json.parseStringAs(text, Content.class); | |||
return OptionalLong.of(content.brokerEpoch); | |||
} catch (Exception e) { | |||
logger.debug("Fail to read the clean shutdown file in " + cleanShutdownFile.toPath() + ":" + e); | |||
logger.debug("Fail to read the clean shutdown file in {}:{}", cleanShutdownFile.toPath(), e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the last parameter could be handled well if it is an exception object. Perhaps, we could use {}
instead of {}:{}
?
result = 31 * result + Long.hashCode(timestamp); | ||
return result; | ||
} | ||
|
||
@Override | ||
public String toString() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not here as this also prints firstSeq
and firstOffset
which are not fields.
result = 31 * result + Boolean.hashCode(isAborted); | ||
return result; | ||
} | ||
public record CompletedTxn(long producerId, long firstOffset, long lastOffset, boolean isAborted) { | ||
|
||
@Override | ||
public String toString() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
IndexValue(T index) { | ||
this.index = index; | ||
} | ||
private record IndexValue<T extends AbstractIndex>(T index) implements IndexWrapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was an argument before for the "suitable" variables in a record class. I prefer to use record class only if all variables are immutable. That can ensure the hashCode
and equals
are consistent. What do you think? That is a kind of personal taste, so any feedback is welcomed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's a bit of a gray area. I'm happy to keep it as a regular class. I undid that change
@mimaison could you please fix the conflicts? |
79eb911
to
ffdc1a1
Compare
Rebased on trunk. The test failure, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mimaison thanks for this cleanup. overall LGTM
if (!file.exists()) { | ||
return true; | ||
} | ||
return file.delete(); | ||
} catch (final Exception e) { | ||
LOGGER.error(format("Encountered error while deleting %s", file.getAbsolutePath())); | ||
LOGGER.error("Encountered error while deleting {}", file.getAbsolutePath()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should add the e
to the logger
LOGGER.error("Encountered error while deleting {}", file.getAbsolutePath(), e);
public Integer getExpectedFromSecondTierCount() { | ||
return expectedFromSecondTierCount; | ||
} | ||
public record ConsumableSpec(Long fetchOffset, Integer expectedTotalCount, Integer expectedFromSecondTierCount) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the usage of this class, and it appears using primitive types should be fine.
public Integer getEventCount() { | ||
return eventCount; | ||
} | ||
public record DeletableSpec(Integer sourceBrokerId, LocalTieredStorageEvent.EventType eventType, Integer eventCount) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
public RemoteFetchCount getFetchCount() { | ||
return fetchCount; | ||
} | ||
public record FetchableSpec(Integer sourceBrokerId, RemoteFetchCount fetchCount) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
public List<ProducerRecord<String, String>> getRecords() { | ||
return records; | ||
} | ||
public record OffloadableSpec(Integer sourceBrokerId, Integer baseOffset, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Cleanups including: