gemmellr commented on code in PR #5487:
URL: https://github.com/apache/activemq-artemis/pull/5487#discussion_r1949035079
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java:
##########
@@ -1380,16 +1380,16 @@ public CompositeData toCompositeData(int fieldsLimit,
int deliveryCount) throws
CompositeType ct;
Map<String, Object> fields;
byte type = getType();
- switch (type) {
- case Message.TEXT_TYPE:
+ fields = switch (type) {
Review Comment:
Would seem to make sense to move the declaration down to the same line as
well for clarity.
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/converter/AMQPMessageSupport.java:
##########
@@ -415,11 +415,11 @@ public static String destination(RoutingType
destinationType, String address) {
destinationType = RoutingType.ANYCAST;
}
- switch (destinationType) {
- case ANYCAST: prefix = QUEUE_QUALIFIED_PREFIX; break;
- case MULTICAST: prefix = TOPIC_QUALIFIED_PREFIX; break;
- default: prefix = QUEUE_QUALIFIED_PREFIX; break;
- }
+ prefix = switch (destinationType) {
Review Comment:
Would make sense to move the declaration down to this same line
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/ConcurrentLongHashMap.java:
##########
@@ -235,7 +235,7 @@ V get(long key, int keyHash) {
if (!acquiredLock && validate(stamp)) {
// The values we have read are consistent
if (storedKey == key) {
- return storedValue != DeletedValue ? storedValue : null;
+ return storedValue == DeletedValue ? null : storedValue;
Review Comment:
I think both of the changes like this in this class read better the way they
were.
##########
artemis-ra/src/main/java/org/apache/activemq/artemis/ra/inflow/ActiveMQActivation.java:
##########
@@ -549,21 +549,15 @@ protected void setupDestination() throws Exception {
throw ActiveMQRABundle.BUNDLE.noDestinationName();
}
+ // If there is no binding on naming, we will just create a new
instance
String calculatedDestinationName =
destinationName.substring(destinationName.lastIndexOf('/') + 1);
if (isTopic) {
- calculatedDestinationName =
getTopicWithPrefix(calculatedDestinationName);
- } else if (!isTopic) {
- calculatedDestinationName =
getQueueWithPrefix(calculatedDestinationName);
+ destination =
ActiveMQDestination.createTopic(getTopicWithPrefix(calculatedDestinationName));
Review Comment:
By not updating the calculatedDestinationName variable as it did previously,
this is changing the value reported by the logging below. Intended, or?
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/RandomUtil.java:
##########
@@ -31,17 +31,56 @@ public static Random getRandom() {
return random;
}
+ private static String letters = "abcdefghijklmnopqrstuvwxyz";
+
+ private static String digits = "0123456789";
+
+ private static String randomBase = letters + letters.toUpperCase() + digits;
+
+ /**
+ * Utility method to build a {@code String} filled with random
alpha-numeric characters. The {@code String} will
+ * contain characters from the following:
+ * <ul>
+ * <li>abcdefghijklmnopqrstuvwxyz</li>
+ * <li>ABCDEFGHIJKLMNOPQRSTUVWXYZ</li>
+ * <li>0123456789</li>
+ * </ul>
+ * @param length how long the returned {@code String} should be
+ * @return a {@code String} of random alpha-numeric characters
+ */
+ public static String randomAlphaNumericString(int length) {
+ StringBuilder result = new StringBuilder(length);
+ for (int i = 0; i < length; i++) {
+ result.append(randomBase.charAt(randomInterval(0,
randomBase.length())));
+ }
+ return result.toString();
+ }
- public static String randomString() {
Review Comment:
Since this is in `artemis-commons` main and is public, this is technically a
breaking change. Whilst I'd hope noone is using it, I would also not be shocked
if so. Might be better/safer to only deprecate this method for now and then
remove it a bit later.
Alternatively, perhaps could maybe even work toward doing that for most/all
of the methods and only have them in the _tests_ tree, rather than leaving them
and deleting the util [sub-]class in the tests tree as you did here?
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/RandomUtil.java:
##########
@@ -31,17 +31,56 @@ public static Random getRandom() {
return random;
}
+ private static String letters = "abcdefghijklmnopqrstuvwxyz";
+
+ private static String digits = "0123456789";
+
+ private static String randomBase = letters + letters.toUpperCase() + digits;
+
+ /**
+ * Utility method to build a {@code String} filled with random
alpha-numeric characters. The {@code String} will
+ * contain characters from the following:
+ * <ul>
+ * <li>abcdefghijklmnopqrstuvwxyz</li>
+ * <li>ABCDEFGHIJKLMNOPQRSTUVWXYZ</li>
+ * <li>0123456789</li>
+ * </ul>
+ * @param length how long the returned {@code String} should be
+ * @return a {@code String} of random alpha-numeric characters
+ */
+ public static String randomAlphaNumericString(int length) {
+ StringBuilder result = new StringBuilder(length);
+ for (int i = 0; i < length; i++) {
+ result.append(randomBase.charAt(randomInterval(0,
randomBase.length())));
Review Comment:
Either the randomInterval method can _never_ pick the second(max?) value
(which seems odd when having 2 args), or this has a bug where it could try to
pick the character 1-beyond the end of randomBase...which is it?
##########
artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java:
##########
@@ -3053,42 +3053,21 @@ private static boolean isContainsBody(final byte
recordType) {
private static int getRecordSize(final byte recordType, final int
journalVersion) {
// The record size (without the variable portion)
- int recordSize = 0;
- switch (recordType) {
- case ADD_RECORD:
- case EVENT_RECORD:
- recordSize = JournalImpl.SIZE_ADD_RECORD;
- break;
- case UPDATE_RECORD:
- recordSize = JournalImpl.SIZE_ADD_RECORD;
- break;
- case ADD_RECORD_TX:
- recordSize = JournalImpl.SIZE_ADD_RECORD_TX;
- break;
- case UPDATE_RECORD_TX:
- recordSize = JournalImpl.SIZE_ADD_RECORD_TX;
- break;
- case DELETE_RECORD:
- recordSize = JournalImpl.SIZE_DELETE_RECORD;
- break;
- case DELETE_RECORD_TX:
- recordSize = JournalImpl.SIZE_DELETE_RECORD_TX;
- break;
- case PREPARE_RECORD:
- recordSize = JournalImpl.SIZE_PREPARE_RECORD;
- break;
- case COMMIT_RECORD:
- recordSize = JournalImpl.SIZE_COMMIT_RECORD;
- break;
- case ROLLBACK_RECORD:
- recordSize = JournalImpl.SIZE_ROLLBACK_RECORD;
- break;
- default:
+ int recordSize = switch (recordType) {
+ case ADD_RECORD, EVENT_RECORD -> JournalImpl.SIZE_ADD_RECORD;
+ case UPDATE_RECORD -> JournalImpl.SIZE_ADD_RECORD;
Review Comment:
These should be the same case if having multi-label for the first 2.
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyConnector.java:
##########
@@ -582,8 +582,8 @@ public synchronized void start() {
if (tcpSendBufferSize != -1) {
bootstrap.option(ChannelOption.SO_SNDBUF, tcpSendBufferSize);
}
- final int writeBufferLowWaterMark = this.writeBufferLowWaterMark != -1 ?
this.writeBufferLowWaterMark : WriteBufferWaterMark.DEFAULT.low();
- final int writeBufferHighWaterMark = this.writeBufferHighWaterMark != -1
? this.writeBufferHighWaterMark : WriteBufferWaterMark.DEFAULT.high();
+ final int writeBufferLowWaterMark = this.writeBufferLowWaterMark == -1 ?
WriteBufferWaterMark.DEFAULT.low() : this.writeBufferLowWaterMark;
Review Comment:
Personally I think this and all the others changes after it in this commit,
are better the way they are. They all check 'is something set / did something
arrive? -> then use it, otherwise <get-default>'
I would drop the whole commit, now that I look at having commented about all
but 1 of the changes :)
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ChannelImpl.java:
##########
@@ -171,34 +171,21 @@ public boolean supports(final byte packetType) {
@Override
public boolean supports(final byte packetType, int version) {
- switch (packetType) {
- case PacketImpl.CLUSTER_TOPOLOGY_V2:
- return version >= 122;
- case PacketImpl.DISCONNECT_CONSUMER:
- return version >= 124;
- case PacketImpl.CLUSTER_TOPOLOGY_V3:
- return version >= 125;
- case PacketImpl.DISCONNECT_V2:
- return version >= 125;
- case PacketImpl.SESS_QUEUEQUERY_RESP_V2:
- return version >= 126;
- case PacketImpl.SESS_BINDINGQUERY_RESP_V2:
- return version >= 126;
- case PacketImpl.SESS_BINDINGQUERY_RESP_V3:
- return version >= 127;
- case PacketImpl.SESS_QUEUEQUERY_RESP_V3:
- return version >= 129;
- case PacketImpl.SESS_BINDINGQUERY_RESP_V4:
- return version >= 129;
- case PacketImpl.CLUSTER_TOPOLOGY_V4:
- case PacketImpl.CREATESESSION_V2:
- case PacketImpl.DISCONNECT_V3:
- return version >= PacketImpl.ARTEMIS_2_18_0_VERSION;
- case PacketImpl.SESS_BINDINGQUERY_RESP_V5:
- return version >= PacketImpl.ARTEMIS_2_29_0_VERSION;
- default:
- return true;
- }
+ return switch (packetType) {
+ case PacketImpl.CLUSTER_TOPOLOGY_V2 -> version >= 122;
+ case PacketImpl.DISCONNECT_CONSUMER -> version >= 124;
+ case PacketImpl.CLUSTER_TOPOLOGY_V3 -> version >= 125;
+ case PacketImpl.DISCONNECT_V2 -> version >= 125;
+ case PacketImpl.SESS_QUEUEQUERY_RESP_V2 -> version >= 126;
+ case PacketImpl.SESS_BINDINGQUERY_RESP_V2 -> version >= 126;
+ case PacketImpl.SESS_BINDINGQUERY_RESP_V3 -> version >= 127;
+ case PacketImpl.SESS_QUEUEQUERY_RESP_V3 -> version >= 129;
+ case PacketImpl.SESS_BINDINGQUERY_RESP_V4 -> version >= 129;
+ case PacketImpl.CLUSTER_TOPOLOGY_V4, PacketImpl.CREATESESSION_V2,
PacketImpl.DISCONNECT_V3 ->
+ version >= PacketImpl.ARTEMIS_2_18_0_VERSION;
Review Comment:
If we are going to have any of the multi-labels here...there are 3 other
skipped/missed opportunities above this (versions >= 125, 126, and 129) to use
them. Should probably consistently use them, or consistently not in any given
switch.
(They also may or may not be a bit less ugly if these were all static
imported?)
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/wireformat/SessionSendContinuationMessage.java:
##########
@@ -115,7 +115,7 @@ public Message getMessage() {
@Override
public int expectedEncodeSize() {
- return super.expectedEncodeSize() + (!continues ?
DataConstants.SIZE_LONG : 0) + DataConstants.SIZE_BOOLEAN;
+ return super.expectedEncodeSize() + (continues ? 0 :
DataConstants.SIZE_LONG) + DataConstants.SIZE_BOOLEAN;
Review Comment:
The original !continues aligns better with the logic in the encoding and
decoding that this result matches up with. I think it would be better left as
it is.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java:
##########
@@ -3438,7 +3438,7 @@ private boolean needsDepage() {
final int prefetchMessages =
pageSubscription.getPagingStore().getPrefetchPageMessages();
final int prefetchBytes =
pageSubscription.getPagingStore().getPrefetchPageBytes();
- if (maxReadMessages <= 0 && maxReadBytes <= 0 && prefetchBytes <= 0 &&
prefetchBytes <= 0) {
+ if (maxReadMessages <= 0 && maxReadBytes <= 0 && prefetchBytes <= 0) {
Review Comment:
Seems as likely this is a c&p bug that should be fixed rather than just a
duplicate that should be deleted, and it should instead be checking
_prefetchMessages_ <=0. The commend notes 'all values being disabled' and this
was a new config added at the time as the prefetchBytes <= 0 check (both
defaulting to -1 in the code at the time, though maybe not XML).
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/RandomUtil.java:
##########
@@ -31,17 +31,56 @@ public static Random getRandom() {
return random;
}
+ private static String letters = "abcdefghijklmnopqrstuvwxyz";
+
+ private static String digits = "0123456789";
+
+ private static String randomBase = letters + letters.toUpperCase() + digits;
+
+ /**
+ * Utility method to build a {@code String} filled with random
alpha-numeric characters. The {@code String} will
+ * contain characters from the following:
+ * <ul>
+ * <li>abcdefghijklmnopqrstuvwxyz</li>
+ * <li>ABCDEFGHIJKLMNOPQRSTUVWXYZ</li>
+ * <li>0123456789</li>
+ * </ul>
+ * @param length how long the returned {@code String} should be
+ * @return a {@code String} of random alpha-numeric characters
+ */
+ public static String randomAlphaNumericString(int length) {
+ StringBuilder result = new StringBuilder(length);
+ for (int i = 0; i < length; i++) {
+ result.append(randomBase.charAt(randomInterval(0,
randomBase.length())));
+ }
+ return result.toString();
+ }
- public static String randomString() {
+ /**
+ * @return A randomly generated {@link java.util.UUID} converted to a
{@code String}
+ */
+ public static String randomUUIDString() {
return java.util.UUID.randomUUID().toString();
}
- public static SimpleString randomSimpleString() {
Review Comment:
Ditto
##########
artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/RunnableCallback.java:
##########
Review Comment:
I don't immediately see anything using this class so perhaps it should go
rather than being fixed.
##########
artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/impl/JournalImpl.java:
##########
@@ -3053,42 +3053,21 @@ private static boolean isContainsBody(final byte
recordType) {
private static int getRecordSize(final byte recordType, final int
journalVersion) {
// The record size (without the variable portion)
- int recordSize = 0;
- switch (recordType) {
- case ADD_RECORD:
- case EVENT_RECORD:
- recordSize = JournalImpl.SIZE_ADD_RECORD;
- break;
- case UPDATE_RECORD:
- recordSize = JournalImpl.SIZE_ADD_RECORD;
- break;
- case ADD_RECORD_TX:
- recordSize = JournalImpl.SIZE_ADD_RECORD_TX;
- break;
- case UPDATE_RECORD_TX:
- recordSize = JournalImpl.SIZE_ADD_RECORD_TX;
- break;
- case DELETE_RECORD:
- recordSize = JournalImpl.SIZE_DELETE_RECORD;
- break;
- case DELETE_RECORD_TX:
- recordSize = JournalImpl.SIZE_DELETE_RECORD_TX;
- break;
- case PREPARE_RECORD:
- recordSize = JournalImpl.SIZE_PREPARE_RECORD;
- break;
- case COMMIT_RECORD:
- recordSize = JournalImpl.SIZE_COMMIT_RECORD;
- break;
- case ROLLBACK_RECORD:
- recordSize = JournalImpl.SIZE_ROLLBACK_RECORD;
- break;
- default:
+ int recordSize = switch (recordType) {
+ case ADD_RECORD, EVENT_RECORD -> JournalImpl.SIZE_ADD_RECORD;
+ case UPDATE_RECORD -> JournalImpl.SIZE_ADD_RECORD;
+ case ADD_RECORD_TX -> JournalImpl.SIZE_ADD_RECORD_TX;
+ case UPDATE_RECORD_TX -> JournalImpl.SIZE_ADD_RECORD_TX;
Review Comment:
These _could_ use the same case, and probably should if already having
multi-labels earlier in the switch.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/replication/ReplicationManager.java:
##########
@@ -737,8 +732,7 @@ private void
sendLargeFile(AbstractJournalStorageManager.JournalContent content,
final ReusableLatch flushed = new ReusableLatch(1);
try {
- try (FileInputStream fis = new FileInputStream(file.getJavaFile());
- FileChannel channel = fis.getChannel()) {
+ try (FileInputStream fis = new FileInputStream(file.getJavaFile());
FileChannel channel = fis.getChannel()) {
Review Comment:
This is both unrelated to the commits sole subject of updating switch
statements, and also makes the code less readable to have the two declarations
on the same line rather than their own as before. Better left as it was.
(there are also other unrelated/non-switch changes in the commit for this
class, above and below this point, though they at least dont make things less
readable)
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/RandomUtil.java:
##########
@@ -31,17 +31,56 @@ public static Random getRandom() {
return random;
}
+ private static String letters = "abcdefghijklmnopqrstuvwxyz";
+
+ private static String digits = "0123456789";
+
+ private static String randomBase = letters + letters.toUpperCase() + digits;
+
+ /**
+ * Utility method to build a {@code String} filled with random
alpha-numeric characters. The {@code String} will
+ * contain characters from the following:
+ * <ul>
+ * <li>abcdefghijklmnopqrstuvwxyz</li>
+ * <li>ABCDEFGHIJKLMNOPQRSTUVWXYZ</li>
+ * <li>0123456789</li>
+ * </ul>
+ * @param length how long the returned {@code String} should be
+ * @return a {@code String} of random alpha-numeric characters
+ */
+ public static String randomAlphaNumericString(int length) {
+ StringBuilder result = new StringBuilder(length);
+ for (int i = 0; i < length; i++) {
+ result.append(randomBase.charAt(randomInterval(0,
randomBase.length())));
+ }
+ return result.toString();
+ }
- public static String randomString() {
+ /**
+ * @return A randomly generated {@link java.util.UUID} converted to a
{@code String}
+ */
+ public static String randomUUIDString() {
return java.util.UUID.randomUUID().toString();
}
- public static SimpleString randomSimpleString() {
- return SimpleString.of(RandomUtil.randomString());
+ /**
+ * @return A randomly generated {@link java.util.UUID} converted to a
{@link SimpleString}
+ */
+ public static SimpleString randomUUIDSimpleString() {
+ return SimpleString.of(RandomUtil.randomUUIDString());
}
+ /**
+ * Utility method to get a random alpha-numeric character. The {@code char}
will be one of the following:
+ * <ul>
+ * <li>abcdefghijklmnopqrstuvwxyz</li>
+ * <li>ABCDEFGHIJKLMNOPQRSTUVWXYZ</li>
+ * <li>0123456789</li>
+ * </ul>
+ * @return A randomly generated alpha-numeric {@code char}
+ */
public static char randomChar() {
- return RandomUtil.randomString().charAt(0);
+ return randomBase.charAt(randomInterval(0, randomBase.length()));
Review Comment:
Similar comment to earlier for randomAlphaNumericString(..)
--
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: gitbox-unsubscr...@activemq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org
For additional commands, e-mail: gitbox-h...@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
