Re: [PR] KAFKA-15582: Move the clean shutdown file to the storage package [kafka]
junrao merged PR #14603: URL: https://github.com/apache/kafka/pull/14603 -- 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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on PR #14603: URL: https://github.com/apache/kafka/pull/14603#issuecomment-1786194305 https://issues.apache.org/jira/browse/KAFKA-15759 DescribeClusterRequestTest https://issues.apache.org/jira/browse/KAFKA-15760 CoordinatorTest https://issues.apache.org/jira/browse/KAFKA-15761 ConnectorRestartApiIntegrationTest https://issues.apache.org/jira/browse/KAFKA-15762 ClusterConnectionStatesTest Added a few more common failures. -- 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-15582: Move the clean shutdown file to the storage package [kafka]
junrao closed pull request #14603: KAFKA-15582: Move the clean shutdown file to the storage package URL: https://github.com/apache/kafka/pull/14603 -- 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-15582: Move the clean shutdown file to the storage package [kafka]
junrao commented on PR #14603: URL: https://github.com/apache/kafka/pull/14603#issuecomment-1785851656 @CalvinConfluent : -- 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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on PR #14603: URL: https://github.com/apache/kafka/pull/14603#issuecomment-1783656823 Found wield UT failure, rebase trunk. -- 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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on code in PR #14603:
URL: https://github.com/apache/kafka/pull/14603#discussion_r1374835745
##
storage/src/main/java/org/apache/kafka/storage/internals/checkpoint/CleanShutdownFileHandler.java:
##
@@ -85,17 +86,16 @@ void write(long brokerEpoch, int version) throws Exception {
}
}
-public long read() {
-long brokerEpoch = -1L;
+@SuppressWarnings("unchecked")
+public OptionalLong read() {
try {
String text =
Utils.readFileAsString(cleanShutdownFile.toPath().toString());
-Map content = new ObjectMapper().readValue(text,
HashMap.class);
-
-brokerEpoch =
Long.parseLong(content.getOrDefault(Fields.BROKER_EPOCH.toString(), "-1L"));
+Content content = Json.parseStringAs(text, Content.class);
+return OptionalLong.of(content.brokerEpoch);
} catch (Exception e) {
-logger.warn("Fail to read the clean shutdown file in " +
cleanShutdownFile.toPath() + ":" + e);
+logger.trace("Fail to read the clean shutdown file in " +
cleanShutdownFile.toPath() + ":" + e);
Review Comment:
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-15582: Move the clean shutdown file to the storage package [kafka]
junrao commented on code in PR #14603:
URL: https://github.com/apache/kafka/pull/14603#discussion_r1374791457
##
storage/src/main/java/org/apache/kafka/storage/internals/checkpoint/CleanShutdownFileHandler.java:
##
@@ -85,17 +86,16 @@ void write(long brokerEpoch, int version) throws Exception {
}
}
-public long read() {
-long brokerEpoch = -1L;
+@SuppressWarnings("unchecked")
+public OptionalLong read() {
try {
String text =
Utils.readFileAsString(cleanShutdownFile.toPath().toString());
-Map content = new ObjectMapper().readValue(text,
HashMap.class);
-
-brokerEpoch =
Long.parseLong(content.getOrDefault(Fields.BROKER_EPOCH.toString(), "-1L"));
+Content content = Json.parseStringAs(text, Content.class);
+return OptionalLong.of(content.brokerEpoch);
} catch (Exception e) {
-logger.warn("Fail to read the clean shutdown file in " +
cleanShutdownFile.toPath() + ":" + e);
+logger.trace("Fail to read the clean shutdown file in " +
cleanShutdownFile.toPath() + ":" + e);
Review Comment:
This probably should be debug?
--
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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on PR #14603: URL: https://github.com/apache/kafka/pull/14603#issuecomment-1781972169 https://issues.apache.org/jira/browse/KAFKA-15699 https://issues.apache.org/jira/browse/KAFKA-15700 -- 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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on PR #14603: URL: https://github.com/apache/kafka/pull/14603#issuecomment-1781399982 @junrao The failed tests are irrelevant. Found some flaky integration tests(they can pass locally but take a long time) https://issues.apache.org/jira/browse/KAFKA-15690 -- 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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on code in PR #14603:
URL: https://github.com/apache/kafka/pull/14603#discussion_r1372064203
##
core/src/main/scala/kafka/server/BrokerLifecycleManager.scala:
##
@@ -190,7 +192,7 @@ class BrokerLifecycleManager(
/**
* The broker epoch from the previous run, or -1 if the epoch is not able to
be found.
*/
- @volatile private var previousBrokerEpoch: Long = -1L
+ @volatile private var previousBrokerEpoch: OptionalLong =
OptionalLong.empty()
Review Comment:
Done.
##
storage/src/main/java/org/apache/kafka/storage/internals/checkpoint/CleanShutdownFileHandler.java:
##
@@ -45,17 +44,22 @@
public class CleanShutdownFileHandler {
public static final String CLEAN_SHUTDOWN_FILE_NAME =
".kafka_cleanshutdown";
-private final File cleanShutdownFile;
+// Visible for testing
+public final File cleanShutdownFile;
Review Comment:
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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on code in PR #14603:
URL: https://github.com/apache/kafka/pull/14603#discussion_r1372063969
##
storage/src/main/java/org/apache/kafka/storage/internals/checkpoint/CleanShutdownFileHandler.java:
##
@@ -85,17 +87,16 @@ void write(long brokerEpoch, int version) throws Exception {
}
}
-public long read() {
-long brokerEpoch = -1L;
+@SuppressWarnings("unchecked")
+public OptionalLong read() {
try {
String text =
Utils.readFileAsString(cleanShutdownFile.toPath().toString());
-Map content = new ObjectMapper().readValue(text,
HashMap.class);
-
-brokerEpoch =
Long.parseLong(content.getOrDefault(Fields.BROKER_EPOCH.toString(), "-1L"));
+Content content = OBJECT_MAPPER.readValue(text, Content.class);
Review Comment:
Thanks! Good to know we have a package to do it.
##
storage/src/main/java/org/apache/kafka/storage/internals/checkpoint/CleanShutdownFileHandler.java:
##
@@ -73,10 +77,8 @@ void write(long brokerEpoch, int version) throws Exception {
FileOutputStream os = new FileOutputStream(cleanShutdownFile);
BufferedWriter bw = new BufferedWriter(new OutputStreamWriter(os,
StandardCharsets.UTF_8));
try {
-Map payload = new HashMap<>();
-payload.put(Fields.VERSION.toString(), Integer.toString(version));
-payload.put(Fields.BROKER_EPOCH.toString(),
Long.toString(brokerEpoch));
-bw.write(new ObjectMapper().writeValueAsString(payload));
+Content content = new Content(version, brokerEpoch);
+bw.write(OBJECT_MAPPER.writeValueAsString(content));
Review Comment:
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-15582: Move the clean shutdown file to the storage package [kafka]
junrao commented on code in PR #14603:
URL: https://github.com/apache/kafka/pull/14603#discussion_r1372017036
##
storage/src/main/java/org/apache/kafka/storage/internals/checkpoint/CleanShutdownFileHandler.java:
##
@@ -73,10 +77,8 @@ void write(long brokerEpoch, int version) throws Exception {
FileOutputStream os = new FileOutputStream(cleanShutdownFile);
BufferedWriter bw = new BufferedWriter(new OutputStreamWriter(os,
StandardCharsets.UTF_8));
try {
-Map payload = new HashMap<>();
-payload.put(Fields.VERSION.toString(), Integer.toString(version));
-payload.put(Fields.BROKER_EPOCH.toString(),
Long.toString(brokerEpoch));
-bw.write(new ObjectMapper().writeValueAsString(payload));
+Content content = new Content(version, brokerEpoch);
+bw.write(OBJECT_MAPPER.writeValueAsString(content));
Review Comment:
Could we reuse `Json.encodeAsString`?
##
storage/src/main/java/org/apache/kafka/storage/internals/checkpoint/CleanShutdownFileHandler.java:
##
@@ -85,17 +87,16 @@ void write(long brokerEpoch, int version) throws Exception {
}
}
-public long read() {
-long brokerEpoch = -1L;
+@SuppressWarnings("unchecked")
+public OptionalLong read() {
try {
String text =
Utils.readFileAsString(cleanShutdownFile.toPath().toString());
-Map content = new ObjectMapper().readValue(text,
HashMap.class);
-
-brokerEpoch =
Long.parseLong(content.getOrDefault(Fields.BROKER_EPOCH.toString(), "-1L"));
+Content content = OBJECT_MAPPER.readValue(text, Content.class);
Review Comment:
Could we reuse `Json.parseStringAs`?
##
storage/src/main/java/org/apache/kafka/storage/internals/checkpoint/CleanShutdownFileHandler.java:
##
@@ -45,17 +44,22 @@
public class CleanShutdownFileHandler {
public static final String CLEAN_SHUTDOWN_FILE_NAME =
".kafka_cleanshutdown";
-private final File cleanShutdownFile;
+// Visible for testing
+public final File cleanShutdownFile;
Review Comment:
Should we limit that to package level access?
##
core/src/main/scala/kafka/server/BrokerLifecycleManager.scala:
##
@@ -190,7 +192,7 @@ class BrokerLifecycleManager(
/**
* The broker epoch from the previous run, or -1 if the epoch is not able to
be found.
*/
- @volatile private var previousBrokerEpoch: Long = -1L
+ @volatile private var previousBrokerEpoch: OptionalLong =
OptionalLong.empty()
Review Comment:
Could we change the above comment on -1 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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on code in PR #14603:
URL: https://github.com/apache/kafka/pull/14603#discussion_r1371113716
##
storage/src/main/java/org/apache/kafka/storage/internals/log/CleanShutdownFileHandler.java:
##
@@ -85,17 +83,16 @@ void write(long brokerEpoch, int version) throws Exception {
}
}
-public long read() {
-long brokerEpoch = -1L;
+@SuppressWarnings("unchecked")
+public OptionalLong read() {
try {
String text =
Utils.readFileAsString(cleanShutdownFile.toPath().toString());
-Map content = new ObjectMapper().readValue(text,
HashMap.class);
Review Comment:
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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on code in PR #14603:
URL: https://github.com/apache/kafka/pull/14603#discussion_r1371113599
##
storage/src/main/java/org/apache/kafka/storage/internals/log/CleanShutdownFileHandler.java:
##
@@ -85,17 +83,16 @@ void write(long brokerEpoch, int version) throws Exception {
}
}
-public long read() {
-long brokerEpoch = -1L;
+@SuppressWarnings("unchecked")
+public OptionalLong read() {
try {
String text =
Utils.readFileAsString(cleanShutdownFile.toPath().toString());
-Map content = new ObjectMapper().readValue(text,
HashMap.class);
-
-brokerEpoch =
Long.parseLong(content.getOrDefault(Fields.BROKER_EPOCH.toString(), "-1L"));
+Content content = new ObjectMapper().readValue(text,
Content.class);
Review Comment:
Thanks for the tips!
##
core/src/main/scala/kafka/log/LogManager.scala:
##
@@ -1421,15 +1422,15 @@ class LogManager(logDirs: Seq[File],
for (dir <- liveLogDirs) {
val cleanShutdownFileHandler = new CleanShutdownFileHandler(dir.getPath)
val currentBrokerEpoch = cleanShutdownFileHandler.read
- if (currentBrokerEpoch == -1L) {
+ if (!currentBrokerEpoch.isPresent) {
info(s"Unable to read the broker epoch in ${dir.toString}.")
return -1L
}
- if (brokerEpoch != -1 && currentBrokerEpoch != brokerEpoch) {
+ if (brokerEpoch != -1 && currentBrokerEpoch.getAsLong != brokerEpoch) {
info(s"Found different broker epochs in ${dir.toString}.
Other=$brokerEpoch vs current=$currentBrokerEpoch.")
return -1L
}
- brokerEpoch = currentBrokerEpoch
+ brokerEpoch = currentBrokerEpoch.getAsLong
Review Comment:
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-15582: Move the clean shutdown file to the storage package [kafka]
ijuma commented on code in PR #14603:
URL: https://github.com/apache/kafka/pull/14603#discussion_r1371089424
##
storage/src/main/java/org/apache/kafka/storage/internals/log/CleanShutdownFileHandler.java:
##
@@ -85,17 +83,16 @@ void write(long brokerEpoch, int version) throws Exception {
}
}
-public long read() {
-long brokerEpoch = -1L;
+@SuppressWarnings("unchecked")
+public OptionalLong read() {
try {
String text =
Utils.readFileAsString(cleanShutdownFile.toPath().toString());
-Map content = new ObjectMapper().readValue(text,
HashMap.class);
Review Comment:
ObjectMapper creation is expensive, it should be a static final field
typically.
--
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-15582: Move the clean shutdown file to the storage package [kafka]
ijuma commented on code in PR #14603:
URL: https://github.com/apache/kafka/pull/14603#discussion_r1371088934
##
storage/src/main/java/org/apache/kafka/storage/internals/log/CleanShutdownFileHandler.java:
##
@@ -85,17 +83,16 @@ void write(long brokerEpoch, int version) throws Exception {
}
}
-public long read() {
-long brokerEpoch = -1L;
+@SuppressWarnings("unchecked")
+public OptionalLong read() {
try {
String text =
Utils.readFileAsString(cleanShutdownFile.toPath().toString());
-Map content = new ObjectMapper().readValue(text,
HashMap.class);
-
-brokerEpoch =
Long.parseLong(content.getOrDefault(Fields.BROKER_EPOCH.toString(), "-1L"));
+Content content = new ObjectMapper().readValue(text,
Content.class);
Review Comment:
There is both @JsonIgnoreProperties(ignoreUnknown) and @JsonIgnore.
--
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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on code in PR #14603:
URL: https://github.com/apache/kafka/pull/14603#discussion_r1371084764
##
storage/src/main/java/org/apache/kafka/storage/internals/log/CleanShutdownFileHandler.java:
##
@@ -85,17 +83,16 @@ void write(long brokerEpoch, int version) throws Exception {
}
}
-public long read() {
-long brokerEpoch = -1L;
+@SuppressWarnings("unchecked")
+public OptionalLong read() {
try {
String text =
Utils.readFileAsString(cleanShutdownFile.toPath().toString());
-Map content = new ObjectMapper().readValue(text,
HashMap.class);
-
-brokerEpoch =
Long.parseLong(content.getOrDefault(Fields.BROKER_EPOCH.toString(), "-1L"));
+Content content = new ObjectMapper().readValue(text,
Content.class);
Review Comment:
You are right, downgrade is not flexible here. New fields will break the
parser. Will revert the change.
--
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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on code in PR #14603:
URL: https://github.com/apache/kafka/pull/14603#discussion_r1371084404
##
storage/src/main/java/org/apache/kafka/storage/internals/log/CleanShutdownFileHandler.java:
##
@@ -49,13 +47,15 @@ public class CleanShutdownFileHandler {
private static final int CURRENT_VERSION = 0;
private final Logger logger;
-private enum Fields {
-VERSION,
-BROKER_EPOCH;
+private static class Content {
+public int version;
+public Long brokerEpoch;
-@Override
-public String toString() {
-return name().toLowerCase(Locale.ROOT);
+public Content() {};
Review Comment:
This is the constructor used by ObjectMapper().readValue
--
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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on code in PR #14603: URL: https://github.com/apache/kafka/pull/14603#discussion_r1371084249 ## storage/src/main/java/org/apache/kafka/storage/internals/log/CleanShutdownFileHandler.java: ## @@ -15,7 +15,7 @@ * limitations under the License. */ -package kafka.log; +package org.apache.kafka.storage.internals.log; Review Comment: Will do. -- 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-15582: Move the clean shutdown file to the storage package [kafka]
junrao commented on code in PR #14603:
URL: https://github.com/apache/kafka/pull/14603#discussion_r1370850704
##
storage/src/main/java/org/apache/kafka/storage/internals/log/CleanShutdownFileHandler.java:
##
@@ -15,7 +15,7 @@
* limitations under the License.
*/
-package kafka.log;
+package org.apache.kafka.storage.internals.log;
Review Comment:
CleanShutdownFileHandler is a kind of checkpoint file. Would it be better to
have this in the checkpoint package?
##
core/src/main/scala/kafka/log/LogManager.scala:
##
@@ -1421,15 +1422,15 @@ class LogManager(logDirs: Seq[File],
for (dir <- liveLogDirs) {
val cleanShutdownFileHandler = new CleanShutdownFileHandler(dir.getPath)
val currentBrokerEpoch = cleanShutdownFileHandler.read
- if (currentBrokerEpoch == -1L) {
+ if (!currentBrokerEpoch.isPresent) {
info(s"Unable to read the broker epoch in ${dir.toString}.")
return -1L
}
- if (brokerEpoch != -1 && currentBrokerEpoch != brokerEpoch) {
+ if (brokerEpoch != -1 && currentBrokerEpoch.getAsLong != brokerEpoch) {
info(s"Found different broker epochs in ${dir.toString}.
Other=$brokerEpoch vs current=$currentBrokerEpoch.")
return -1L
}
- brokerEpoch = currentBrokerEpoch
+ brokerEpoch = currentBrokerEpoch.getAsLong
Review Comment:
Should we propagate Option[Long] all the way until we convert it to a
request?
##
storage/src/main/java/org/apache/kafka/storage/internals/log/CleanShutdownFileHandler.java:
##
@@ -85,17 +83,16 @@ void write(long brokerEpoch, int version) throws Exception {
}
}
-public long read() {
-long brokerEpoch = -1L;
+@SuppressWarnings("unchecked")
+public OptionalLong read() {
try {
String text =
Utils.readFileAsString(cleanShutdownFile.toPath().toString());
-Map content = new ObjectMapper().readValue(text,
HashMap.class);
-
-brokerEpoch =
Long.parseLong(content.getOrDefault(Fields.BROKER_EPOCH.toString(), "-1L"));
+Content content = new ObjectMapper().readValue(text,
Content.class);
Review Comment:
Hmm, by doing this, it seems that we can't upgrade from the current clean
shutdown file nor could we downgrade if we add a new field in the future?
##
storage/src/main/java/org/apache/kafka/storage/internals/log/CleanShutdownFileHandler.java:
##
@@ -49,13 +47,15 @@ public class CleanShutdownFileHandler {
private static final int CURRENT_VERSION = 0;
private final Logger logger;
-private enum Fields {
-VERSION,
-BROKER_EPOCH;
+private static class Content {
+public int version;
+public Long brokerEpoch;
-@Override
-public String toString() {
-return name().toLowerCase(Locale.ROOT);
+public Content() {};
Review Comment:
Is this constructor used?
--
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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on code in PR #14603:
URL: https://github.com/apache/kafka/pull/14603#discussion_r1370658110
##
storage/src/main/java/org/apache/kafka/storage/internals/util/CleanShutdownFileHandler.java:
##
@@ -85,6 +85,7 @@ void write(long brokerEpoch, int version) throws Exception {
}
}
+@SuppressWarnings("unchecked")
public long read() {
long brokerEpoch = -1L;
Review Comment:
Thanks for the advice, updated.
--
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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on code in PR #14603: URL: https://github.com/apache/kafka/pull/14603#discussion_r1370657796 ## storage/src/main/java/org/apache/kafka/storage/internals/util/CleanShutdownFileHandler.java: ## @@ -15,7 +15,7 @@ * limitations under the License. */ -package kafka.log; +package org.apache.kafka.storage.internals.util; Review Comment: Done. Moved to log -- 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-15582: Move the clean shutdown file to the storage package [kafka]
ijuma commented on code in PR #14603:
URL: https://github.com/apache/kafka/pull/14603#discussion_r1369015980
##
storage/src/main/java/org/apache/kafka/storage/internals/util/CleanShutdownFileHandler.java:
##
@@ -85,6 +85,7 @@ void write(long brokerEpoch, int version) throws Exception {
}
}
+@SuppressWarnings("unchecked")
public long read() {
long brokerEpoch = -1L;
Review Comment:
A few comments unrelated to this PR:
1. Should we return `OptionalInt` to make it clear that it can be unset?
2. Should we use a class instead of map to avoid the need to map it back and
forth?
--
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-15582: Move the clean shutdown file to the storage package [kafka]
ijuma commented on code in PR #14603: URL: https://github.com/apache/kafka/pull/14603#discussion_r1369013666 ## storage/src/main/java/org/apache/kafka/storage/internals/util/CleanShutdownFileHandler.java: ## @@ -15,7 +15,7 @@ * limitations under the License. */ -package kafka.log; +package org.apache.kafka.storage.internals.util; Review Comment: I was asking if it would fit under one of the existing pcakages: `checkpoint` or `log`. ## storage/src/main/java/org/apache/kafka/storage/internals/util/CleanShutdownFileHandler.java: ## @@ -15,7 +15,7 @@ * limitations under the License. */ -package kafka.log; +package org.apache.kafka.storage.internals.util; Review Comment: I was asking if it would fit under one of the existing packages: `checkpoint` or `log`. -- 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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on code in PR #14603: URL: https://github.com/apache/kafka/pull/14603#discussion_r1369010124 ## storage/src/main/java/org/apache/kafka/storage/internals/util/CleanShutdownFileHandler.java: ## @@ -15,7 +15,7 @@ * limitations under the License. */ -package kafka.log; +package org.apache.kafka.storage.internals.util; Review Comment: Do you mean it should have its own package? Or any other place it put 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-15582: Move the clean shutdown file to the storage package [kafka]
ijuma commented on code in PR #14603: URL: https://github.com/apache/kafka/pull/14603#discussion_r1369008762 ## storage/src/main/java/org/apache/kafka/storage/internals/util/CleanShutdownFileHandler.java: ## @@ -15,7 +15,7 @@ * limitations under the License. */ -package kafka.log; +package org.apache.kafka.storage.internals.util; Review Comment: Why did we put this under `util`? -- 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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent commented on PR #14603: URL: https://github.com/apache/kafka/pull/14603#issuecomment-1775608992 @junrao @ijuma Now the build success with irrelevant test failures. -- 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-15582: Move the clean shutdown file to the storage package [kafka]
ijuma commented on PR #14603: URL: https://github.com/apache/kafka/pull/14603#issuecomment-1774106835 Build is failing due to unused imports. -- 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-15582: Move the clean shutdown file to the storage package [kafka]
CalvinConfluent opened a new pull request, #14603: URL: https://github.com/apache/kafka/pull/14603 A follow-up change to move the clean shutdown file to the storage package. -- 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
