ARTEMIS-5769 fix test failures

[jbertram]

No description provided.

[gemmellr]

I dont really 'get' this change, or more the previous one that broke the test.

This was known to be a fixed 0 integer before your previous change, but now all it checks is its got a non-empty string value. Was the prior 0 reflective of not-delivering or not-acknowledging ? Would an empty value / absence not be more reflective of that? From the other change I guess this a string saying 1970 etc for the epoch? Not sure it feels like an improvement, especially as its a breaking change on top.

[jbertram]

I dont really 'get' this change, or more the previous one that broke the test.

So...The idea here was two-fold

1. make all of the time values reported to the web console consistent
2. avoid creating unnecessary instances of java.util.Date

Before 0500c4e some values created via Date#toString() and some were just long values, i.e.:

• Connection
  • creationTime: Date#toString()
• Session
  • creationTime: Date#toString()
• Consumer
  • creationTime: Date#toString()
  • lastDeliveredTime: long
  • lastAcknowledgedTime: long
• Producer
  • creationTime: long

Now, all these values are explicitly and consistently formatted using java.time.format.DateTimeFormatter#RFC_1123_DATE_TIME. Although, one could make an argument that they should all essentially be long values instead so that the ultimate formatting could be more flexible.

To your point about this being a breaking change...Technically speaking, it is a breaking change, but these management methods and the values that they return were designed and added to the API specifically for use by the web console and the web console is made for humans, not machines. Therefore, in my opinion, the same rules don't apply for such changes.

This was known to be a fixed 0 integer before your previous change, but now all it checks is its got a non-empty string value.

That's correct. This is in line with the checks that are being performed for the other attributes, e.g.:

assertNotEquals("", jsonConsumer.getString(ConsumerField.CREATION_TIME.getName()), "creationTime");

The test here is just for presence, not a specific value.

Was the prior 0 reflective of not-delivering or not-acknowledging ?

Yes.

Would an empty value / absence not be more reflective of that?

An explicit indication of no-delivery and/or no-acknowledgement would probably be helpful, although I'm thinking something like like never since this value is meant to be read by humans on the web console.

From the other change I guess this a string saying 1970 etc for the epoch?

Correct.

Not sure it feels like an improvement, especially as its a breaking change on top.

Currently what folks see on the web console for "Last Delivery Time" would be something like 1765400556 which isn't very helpful. The user would have to convert the value himself to get something meaningful. The changes in 0500c4e fix that.

To be clear, I'm not opposed to reverting 0500c4e and doing something else to improve the user experience on the web console. My aim here was simply to explain the rationale for the existing change and answer your specific questions.

Feedback welcome!

[brusdev]

Some users also use the management API via the jolokia endpoint in JSON format. The most commonly used JSON alternatives for timestamps are ISO 8601 strings and Unix epochs, because they are easier to parse.

I'm in favor of moving the final output formatting to the web console because it would allow applying the user's local settings to the output format.

[gemmellr]

Similarly, I think ISO-8601 or epoch millis with local formatting would probably be a better idea since they are easier to parse or more adaptable.

These values may have been added for the console but I fully expect other uses are in play. In either case, I don't think the first Apache Artemis transitional release is the best time to make a breaking change.

I'm going to revert 0500c4e from #6079 while we decide what is done and when.