Re: [PR] KAFKA-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
gharris1727 commented on PR #15620: URL: https://github.com/apache/kafka/pull/15620#issuecomment-2097110125 Thanks @linu-shibu for your patience, I was waiting for something else. -- 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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
gharris1727 merged PR #15620: URL: https://github.com/apache/kafka/pull/15620 -- 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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
linu-shibu commented on PR #15620: URL: https://github.com/apache/kafka/pull/15620#issuecomment-2096976398 @gharris1727 any update on 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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
linu-shibu commented on PR #15620: URL: https://github.com/apache/kafka/pull/15620#issuecomment-2087468237 @gharris1727 I do not have permission/write access to merge the PR. Will I get permission/right to merge? -- 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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
linu-shibu commented on PR #15620: URL: https://github.com/apache/kafka/pull/15620#issuecomment-2087467721 > Test failures appear unrelated, there's a targeted RemoteLogMetadataSerdeTest for this logic, and the storage tests appear to pass for me locally. Yes, in local, the tests are passing for me 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
Re: [PR] KAFKA-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
gharris1727 commented on PR #15620: URL: https://github.com/apache/kafka/pull/15620#issuecomment-2083373419 Test failures appear unrelated, there's a targeted RemoteLogMetadataSerdeTest for this logic, and the storage tests appear to pass for me locally. -- 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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
linu-shibu commented on PR #15620: URL: https://github.com/apache/kafka/pull/15620#issuecomment-2081350906 @gharris1727 any other changes to be made for this PR? Could you review again -- 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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
linu-shibu commented on PR #15620: URL: https://github.com/apache/kafka/pull/15620#issuecomment-2068156085 > Thanks @linu-shibu this is a lot closer to what I expected. > > Can you also add the build.gradle patch I mentioned earlier? I think this is the only raw types used in the storage project. Done, I was under the impression that it was for debugging and fixing the warning in local -- 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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
gharris1727 commented on code in PR #15620:
URL: https://github.com/apache/kafka/pull/15620#discussion_r1573875832
##
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##
@@ -46,69 +44,65 @@ public class RemoteLogMetadataSerde {
private static final short REMOTE_PARTITION_DELETE_API_KEY = new
RemotePartitionDeleteMetadataRecord().apiKey();
private static final short REMOTE_LOG_SEGMENT_METADATA_SNAPSHOT_API_KEY =
new RemoteLogSegmentMetadataSnapshotRecord().apiKey();
-private final Map remoteLogStorageClassToApiKey;
-private final Map keyToTransform;
private final BytesApiMessageSerde bytesApiMessageSerde;
+private ApiMessageAndVersion apiMessageAndVersion;
Review Comment:
I meant it should be a local variable.
--
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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
linu-shibu commented on code in PR #15620:
URL: https://github.com/apache/kafka/pull/15620#discussion_r1573871668
##
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##
@@ -46,69 +44,65 @@ public class RemoteLogMetadataSerde {
private static final short REMOTE_PARTITION_DELETE_API_KEY = new
RemotePartitionDeleteMetadataRecord().apiKey();
private static final short REMOTE_LOG_SEGMENT_METADATA_SNAPSHOT_API_KEY =
new RemoteLogSegmentMetadataSnapshotRecord().apiKey();
-private final Map remoteLogStorageClassToApiKey;
-private final Map keyToTransform;
private final BytesApiMessageSerde bytesApiMessageSerde;
+private ApiMessageAndVersion apiMessageAndVersion;
+
+private final RemoteLogSegmentMetadataTransform
remoteLogSegmentMetadataTransform;
Review Comment:
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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
linu-shibu commented on code in PR #15620:
URL: https://github.com/apache/kafka/pull/15620#discussion_r1573870979
##
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##
@@ -46,69 +44,65 @@ public class RemoteLogMetadataSerde {
private static final short REMOTE_PARTITION_DELETE_API_KEY = new
RemotePartitionDeleteMetadataRecord().apiKey();
private static final short REMOTE_LOG_SEGMENT_METADATA_SNAPSHOT_API_KEY =
new RemoteLogSegmentMetadataSnapshotRecord().apiKey();
-private final Map remoteLogStorageClassToApiKey;
-private final Map keyToTransform;
private final BytesApiMessageSerde bytesApiMessageSerde;
+private ApiMessageAndVersion apiMessageAndVersion;
Review Comment:
Okay, updating this to a class variable
--
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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
gharris1727 commented on code in PR #15620:
URL: https://github.com/apache/kafka/pull/15620#discussion_r1573855150
##
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##
@@ -46,69 +44,65 @@ public class RemoteLogMetadataSerde {
private static final short REMOTE_PARTITION_DELETE_API_KEY = new
RemotePartitionDeleteMetadataRecord().apiKey();
private static final short REMOTE_LOG_SEGMENT_METADATA_SNAPSHOT_API_KEY =
new RemoteLogSegmentMetadataSnapshotRecord().apiKey();
-private final Map remoteLogStorageClassToApiKey;
-private final Map keyToTransform;
private final BytesApiMessageSerde bytesApiMessageSerde;
+private ApiMessageAndVersion apiMessageAndVersion;
+
+private final RemoteLogSegmentMetadataTransform
remoteLogSegmentMetadataTransform;
Review Comment:
This looks pretty verbose, can you shorten the variable names? We're already
in the `RemoteLogMetadataSerde`, so `remoteLog` and `Metadata` end up being
visual clutter.
##
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##
@@ -46,69 +44,65 @@ public class RemoteLogMetadataSerde {
private static final short REMOTE_PARTITION_DELETE_API_KEY = new
RemotePartitionDeleteMetadataRecord().apiKey();
private static final short REMOTE_LOG_SEGMENT_METADATA_SNAPSHOT_API_KEY =
new RemoteLogSegmentMetadataSnapshotRecord().apiKey();
-private final Map remoteLogStorageClassToApiKey;
-private final Map keyToTransform;
private final BytesApiMessageSerde bytesApiMessageSerde;
+private ApiMessageAndVersion apiMessageAndVersion;
Review Comment:
This shouldn't be an instance variable.
--
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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
linu-shibu commented on code in PR #15620:
URL: https://github.com/apache/kafka/pull/15620#discussion_r1573854456
##
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##
@@ -65,50 +59,45 @@ protected ApiMessage newApiMessage(short apiKey) {
return MetadataRecordType.fromId(apiKey).newMetadataRecord();
}
-protected final Map
createRemoteLogMetadataTransforms() {
-Map map = new HashMap<>();
-map.put(REMOTE_LOG_SEGMENT_METADATA_API_KEY, new
RemoteLogSegmentMetadataTransform());
-map.put(REMOTE_LOG_SEGMENT_METADATA_UPDATE_API_KEY, new
RemoteLogSegmentMetadataUpdateTransform());
-map.put(REMOTE_PARTITION_DELETE_API_KEY, new
RemotePartitionDeleteMetadataTransform());
-map.put(REMOTE_LOG_SEGMENT_METADATA_SNAPSHOT_API_KEY, new
RemoteLogSegmentMetadataSnapshotTransform());
-return map;
-}
-
-protected final Map
createRemoteLogStorageClassToApiKeyMap() {
-Map map = new HashMap<>();
-map.put(RemoteLogSegmentMetadata.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_API_KEY);
-map.put(RemoteLogSegmentMetadataUpdate.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_UPDATE_API_KEY);
-map.put(RemotePartitionDeleteMetadata.class.getName(),
REMOTE_PARTITION_DELETE_API_KEY);
-map.put(RemoteLogSegmentMetadataSnapshot.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_SNAPSHOT_API_KEY);
-return map;
-}
-
public byte[] serialize(RemoteLogMetadata remoteLogMetadata) {
-Short apiKey =
remoteLogStorageClassToApiKey.get(remoteLogMetadata.getClass().getName());
-if (apiKey == null) {
-throw new IllegalArgumentException("ApiKey for given
RemoteStorageMetadata class: " + remoteLogMetadata.getClass()
- + " does not exist.");
-}
-@SuppressWarnings("unchecked")
-ApiMessageAndVersion apiMessageAndVersion =
remoteLogMetadataTransform(apiKey).toApiMessageAndVersion(remoteLogMetadata);
+ApiMessageAndVersion apiMessageAndVersion;
+if (remoteLogMetadata instanceof RemoteLogSegmentMetadata) {
+RemoteLogSegmentMetadataTransform metadataTransform = new
RemoteLogSegmentMetadataTransform();
Review Comment:
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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
gharris1727 commented on code in PR #15620:
URL: https://github.com/apache/kafka/pull/15620#discussion_r1573784112
##
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##
@@ -65,50 +59,45 @@ protected ApiMessage newApiMessage(short apiKey) {
return MetadataRecordType.fromId(apiKey).newMetadataRecord();
}
-protected final Map
createRemoteLogMetadataTransforms() {
-Map map = new HashMap<>();
-map.put(REMOTE_LOG_SEGMENT_METADATA_API_KEY, new
RemoteLogSegmentMetadataTransform());
-map.put(REMOTE_LOG_SEGMENT_METADATA_UPDATE_API_KEY, new
RemoteLogSegmentMetadataUpdateTransform());
-map.put(REMOTE_PARTITION_DELETE_API_KEY, new
RemotePartitionDeleteMetadataTransform());
-map.put(REMOTE_LOG_SEGMENT_METADATA_SNAPSHOT_API_KEY, new
RemoteLogSegmentMetadataSnapshotTransform());
-return map;
-}
-
-protected final Map
createRemoteLogStorageClassToApiKeyMap() {
-Map map = new HashMap<>();
-map.put(RemoteLogSegmentMetadata.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_API_KEY);
-map.put(RemoteLogSegmentMetadataUpdate.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_UPDATE_API_KEY);
-map.put(RemotePartitionDeleteMetadata.class.getName(),
REMOTE_PARTITION_DELETE_API_KEY);
-map.put(RemoteLogSegmentMetadataSnapshot.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_SNAPSHOT_API_KEY);
-return map;
-}
-
public byte[] serialize(RemoteLogMetadata remoteLogMetadata) {
-Short apiKey =
remoteLogStorageClassToApiKey.get(remoteLogMetadata.getClass().getName());
-if (apiKey == null) {
-throw new IllegalArgumentException("ApiKey for given
RemoteStorageMetadata class: " + remoteLogMetadata.getClass()
- + " does not exist.");
-}
-@SuppressWarnings("unchecked")
-ApiMessageAndVersion apiMessageAndVersion =
remoteLogMetadataTransform(apiKey).toApiMessageAndVersion(remoteLogMetadata);
+ApiMessageAndVersion apiMessageAndVersion;
+if (remoteLogMetadata instanceof RemoteLogSegmentMetadata) {
+RemoteLogSegmentMetadataTransform metadataTransform = new
RemoteLogSegmentMetadataTransform();
Review Comment:
Please move these variables to fields, and the `new` calls to the serde
constructor. We can reuse the same fields for the deserialization.
--
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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
linu-shibu commented on code in PR #15620:
URL: https://github.com/apache/kafka/pull/15620#discussion_r1573755412
##
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##
@@ -74,25 +72,24 @@ protected final Map
createRemoteLogMetadataTr
return map;
}
-protected final Map
createRemoteLogStorageClassToApiKeyMap() {
-Map map = new HashMap<>();
-map.put(RemoteLogSegmentMetadata.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_API_KEY);
-map.put(RemoteLogSegmentMetadataUpdate.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_UPDATE_API_KEY);
-map.put(RemotePartitionDeleteMetadata.class.getName(),
REMOTE_PARTITION_DELETE_API_KEY);
-map.put(RemoteLogSegmentMetadataSnapshot.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_SNAPSHOT_API_KEY);
-return map;
-}
-
public byte[] serialize(RemoteLogMetadata remoteLogMetadata) {
-Short apiKey =
remoteLogStorageClassToApiKey.get(remoteLogMetadata.getClass().getName());
-if (apiKey == null) {
-throw new IllegalArgumentException("ApiKey for given
RemoteStorageMetadata class: " + remoteLogMetadata.getClass()
- + " does not exist.");
-}
-@SuppressWarnings("unchecked")
-ApiMessageAndVersion apiMessageAndVersion =
remoteLogMetadataTransform(apiKey).toApiMessageAndVersion(remoteLogMetadata);
+RemoteLogMetadataTransform metadataTransform;
+
+if(remoteLogMetadata.getClass() == RemoteLogSegmentMetadata.class) {
Review Comment:
Okay, thanks for the clarification! I will change the comparison to use
instanceof operator since it is already used in the project.
--
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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
gharris1727 commented on code in PR #15620:
URL: https://github.com/apache/kafka/pull/15620#discussion_r1573729186
##
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##
@@ -74,25 +72,24 @@ protected final Map
createRemoteLogMetadataTr
return map;
}
-protected final Map
createRemoteLogStorageClassToApiKeyMap() {
-Map map = new HashMap<>();
-map.put(RemoteLogSegmentMetadata.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_API_KEY);
-map.put(RemoteLogSegmentMetadataUpdate.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_UPDATE_API_KEY);
-map.put(RemotePartitionDeleteMetadata.class.getName(),
REMOTE_PARTITION_DELETE_API_KEY);
-map.put(RemoteLogSegmentMetadataSnapshot.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_SNAPSHOT_API_KEY);
-return map;
-}
-
public byte[] serialize(RemoteLogMetadata remoteLogMetadata) {
-Short apiKey =
remoteLogStorageClassToApiKey.get(remoteLogMetadata.getClass().getName());
-if (apiKey == null) {
-throw new IllegalArgumentException("ApiKey for given
RemoteStorageMetadata class: " + remoteLogMetadata.getClass()
- + " does not exist.");
-}
-@SuppressWarnings("unchecked")
-ApiMessageAndVersion apiMessageAndVersion =
remoteLogMetadataTransform(apiKey).toApiMessageAndVersion(remoteLogMetadata);
+RemoteLogMetadataTransform metadataTransform;
+
+if(remoteLogMetadata.getClass() == RemoteLogSegmentMetadata.class) {
Review Comment:
I don't think it makes a correctness difference since this is a flat
hierarchy and there aren't any subclasses of each of the serialized types.
There might be a slight performance difference, but not significant when the
code is overall unoptimized, and we can't know without measuring it.
I would have reached for instance of because it is more null safe, but if
there's an explicit null guard somewhere else then there is again no difference
in correctness, just style.
--
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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
linu-shibu commented on code in PR #15620:
URL: https://github.com/apache/kafka/pull/15620#discussion_r1573721007
##
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##
@@ -74,25 +72,24 @@ protected final Map
createRemoteLogMetadataTr
return map;
}
-protected final Map
createRemoteLogStorageClassToApiKeyMap() {
-Map map = new HashMap<>();
-map.put(RemoteLogSegmentMetadata.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_API_KEY);
-map.put(RemoteLogSegmentMetadataUpdate.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_UPDATE_API_KEY);
-map.put(RemotePartitionDeleteMetadata.class.getName(),
REMOTE_PARTITION_DELETE_API_KEY);
-map.put(RemoteLogSegmentMetadataSnapshot.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_SNAPSHOT_API_KEY);
-return map;
-}
-
public byte[] serialize(RemoteLogMetadata remoteLogMetadata) {
-Short apiKey =
remoteLogStorageClassToApiKey.get(remoteLogMetadata.getClass().getName());
-if (apiKey == null) {
-throw new IllegalArgumentException("ApiKey for given
RemoteStorageMetadata class: " + remoteLogMetadata.getClass()
- + " does not exist.");
-}
-@SuppressWarnings("unchecked")
-ApiMessageAndVersion apiMessageAndVersion =
remoteLogMetadataTransform(apiKey).toApiMessageAndVersion(remoteLogMetadata);
+RemoteLogMetadataTransform metadataTransform;
+
+if(remoteLogMetadata.getClass() == RemoteLogSegmentMetadata.class) {
Review Comment:
Is there an advantage in using instanceof instead of using getClass() in
this scenario?
--
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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
The-Gamer-01 commented on code in PR #15620:
URL: https://github.com/apache/kafka/pull/15620#discussion_r1573677249
##
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##
@@ -74,25 +72,24 @@ protected final Map
createRemoteLogMetadataTr
return map;
}
-protected final Map
createRemoteLogStorageClassToApiKeyMap() {
-Map map = new HashMap<>();
-map.put(RemoteLogSegmentMetadata.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_API_KEY);
-map.put(RemoteLogSegmentMetadataUpdate.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_UPDATE_API_KEY);
-map.put(RemotePartitionDeleteMetadata.class.getName(),
REMOTE_PARTITION_DELETE_API_KEY);
-map.put(RemoteLogSegmentMetadataSnapshot.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_SNAPSHOT_API_KEY);
-return map;
-}
-
public byte[] serialize(RemoteLogMetadata remoteLogMetadata) {
-Short apiKey =
remoteLogStorageClassToApiKey.get(remoteLogMetadata.getClass().getName());
-if (apiKey == null) {
-throw new IllegalArgumentException("ApiKey for given
RemoteStorageMetadata class: " + remoteLogMetadata.getClass()
- + " does not exist.");
-}
-@SuppressWarnings("unchecked")
-ApiMessageAndVersion apiMessageAndVersion =
remoteLogMetadataTransform(apiKey).toApiMessageAndVersion(remoteLogMetadata);
+RemoteLogMetadataTransform metadataTransform;
+
+if(remoteLogMetadata.getClass() == RemoteLogSegmentMetadata.class) {
Review Comment:
Why this pr don't use **instanceof**
--
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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
linu-shibu commented on PR #15620: URL: https://github.com/apache/kafka/pull/15620#issuecomment-2064470343 > Hi @linu-shibu @showuon this still uses raw types, and so is still type-unsafe. Fixing that was my motivation for creating the ticket, sorry for not emphasizing it more. > > I think it should be addressed in this PR rather than a follow-up, because it involves changes to the exact same code. > > One other thought that occurs to me now is that we should store the metadata transform instances, so that we don't incur the cost of constructing them for each call. > > Thanks! Sure @gharris1727, I will address the above mentioned comments and update the PR. Thanks for pointing it out! -- 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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
gharris1727 commented on PR #15620: URL: https://github.com/apache/kafka/pull/15620#issuecomment-2055290090 Hi @linu-shibu @showuon this still uses raw types, and so is still type-unsafe. Fixing that was my motivation for creating the ticket, sorry for not emphasizing it more. I think it should be addressed in this PR rather than a follow-up, because it involves changes to the exact same code. One other thought that occurs to me now is that we should store the metadata transform instances, so that we don't incur the cost of constructing them for each call. Thanks! -- 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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
showuon commented on code in PR #15620:
URL: https://github.com/apache/kafka/pull/15620#discussion_r1544007618
##
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##
@@ -74,25 +72,25 @@ protected final Map
createRemoteLogMetadataTr
return map;
}
-protected final Map
createRemoteLogStorageClassToApiKeyMap() {
-Map map = new HashMap<>();
-map.put(RemoteLogSegmentMetadata.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_API_KEY);
-map.put(RemoteLogSegmentMetadataUpdate.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_UPDATE_API_KEY);
-map.put(RemotePartitionDeleteMetadata.class.getName(),
REMOTE_PARTITION_DELETE_API_KEY);
-map.put(RemoteLogSegmentMetadataSnapshot.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_SNAPSHOT_API_KEY);
-return map;
-}
-
public byte[] serialize(RemoteLogMetadata remoteLogMetadata) {
-Short apiKey =
remoteLogStorageClassToApiKey.get(remoteLogMetadata.getClass().getName());
-if (apiKey == null) {
-throw new IllegalArgumentException("ApiKey for given
RemoteStorageMetadata class: " + remoteLogMetadata.getClass()
- + " does not exist.");
-}
-@SuppressWarnings("unchecked")
-ApiMessageAndVersion apiMessageAndVersion =
remoteLogMetadataTransform(apiKey).toApiMessageAndVersion(remoteLogMetadata);
+String className = remoteLogMetadata.getClass().getName();
+RemoteLogMetadataTransform metadataTransform;
+
+if(className.equals(RemoteLogSegmentMetadata.class.getName())) {
+metadataTransform = new RemoteLogSegmentMetadataTransform();
+} else if
(className.equals(RemoteLogSegmentMetadataUpdate.class.getName())) {
+metadataTransform = new RemoteLogSegmentMetadataUpdateTransform();
+} else if
(className.equals(RemotePartitionDeleteMetadata.class.getName())) {
+metadataTransform = new RemotePartitionDeleteMetadataTransform();
+} else if
(className.equals(RemoteLogSegmentMetadataSnapshot.class.getName())) {
+metadataTransform = new
RemoteLogSegmentMetadataSnapshotTransform();
Review Comment:
+1
--
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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
soarez commented on code in PR #15620:
URL: https://github.com/apache/kafka/pull/15620#discussion_r1543811354
##
storage/src/main/java/org/apache/kafka/server/log/remote/metadata/storage/serialization/RemoteLogMetadataSerde.java:
##
@@ -74,25 +72,25 @@ protected final Map
createRemoteLogMetadataTr
return map;
}
-protected final Map
createRemoteLogStorageClassToApiKeyMap() {
-Map map = new HashMap<>();
-map.put(RemoteLogSegmentMetadata.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_API_KEY);
-map.put(RemoteLogSegmentMetadataUpdate.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_UPDATE_API_KEY);
-map.put(RemotePartitionDeleteMetadata.class.getName(),
REMOTE_PARTITION_DELETE_API_KEY);
-map.put(RemoteLogSegmentMetadataSnapshot.class.getName(),
REMOTE_LOG_SEGMENT_METADATA_SNAPSHOT_API_KEY);
-return map;
-}
-
public byte[] serialize(RemoteLogMetadata remoteLogMetadata) {
-Short apiKey =
remoteLogStorageClassToApiKey.get(remoteLogMetadata.getClass().getName());
-if (apiKey == null) {
-throw new IllegalArgumentException("ApiKey for given
RemoteStorageMetadata class: " + remoteLogMetadata.getClass()
- + " does not exist.");
-}
-@SuppressWarnings("unchecked")
-ApiMessageAndVersion apiMessageAndVersion =
remoteLogMetadataTransform(apiKey).toApiMessageAndVersion(remoteLogMetadata);
+String className = remoteLogMetadata.getClass().getName();
+RemoteLogMetadataTransform metadataTransform;
+
+if(className.equals(RemoteLogSegmentMetadata.class.getName())) {
+metadataTransform = new RemoteLogSegmentMetadataTransform();
+} else if
(className.equals(RemoteLogSegmentMetadataUpdate.class.getName())) {
+metadataTransform = new RemoteLogSegmentMetadataUpdateTransform();
+} else if
(className.equals(RemotePartitionDeleteMetadata.class.getName())) {
+metadataTransform = new RemotePartitionDeleteMetadataTransform();
+} else if
(className.equals(RemoteLogSegmentMetadataSnapshot.class.getName())) {
+metadataTransform = new
RemoteLogSegmentMetadataSnapshotTransform();
Review Comment:
Instead of comparing `remoteLogMetadata.getClass().getName()` with the
various `*.class.getName()`, can we instead just compare the classes directly?
--
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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
gharris1727 commented on PR #15620:
URL: https://github.com/apache/kafka/pull/15620#issuecomment-2026008220
@linu-shibu Could you fix the rawtypes warnings in this class? You can see
them if you make this change in build.gradle:
```diff
options.encoding = 'UTF-8'
options.compilerArgs << "-Xlint:all"
// temporary exclusions until all the warnings are fixed
-if (!project.path.startsWith(":connect"))
+if (!project.path.startsWith(":connect") &&
!project.path.startsWith(":storage"))
options.compilerArgs << "-Xlint:-rawtypes"
options.compilerArgs << "-Xlint:-serial"
options.compilerArgs << "-Xlint:-try"
```
--
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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
linu-shibu commented on PR #15620: URL: https://github.com/apache/kafka/pull/15620#issuecomment-2025930412 @gharris1727 please do take a look and provide some guidance/feedback. Thanks! -- 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-16356 RemoteLogMetadataSerde: Serializer via class-name dispatch removed and replaced with if-elseif-else conditions [kafka]
linu-shibu opened a new pull request, #15620: URL: https://github.com/apache/kafka/pull/15620 RemoteLogMetadata object, and has to dispatch to one of four serializers depending on it's type which is currently done by taking the class name of the RemoteLogMetadata and looking it up in maps to find the corresponding serializer for that class. This later requires an unchecked cast, because the RemoteLogMetadataTransform is generic. This is replaced by if-elseif-else statements which are type-safe. Map lookup is also removed in this new implementation which might make process faster as mentioned in the ticket. ### 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
