You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "sk0x50 (via GitHub)" <gi...@apache.org> on 2023/05/25 17:33:45 UTC
[GitHub] [ignite-3] sk0x50 opened a new pull request, #2107: IGNITE-19534 Fixed error code duplicating in error message
sk0x50 opened a new pull request, #2107:
URL: https://github.com/apache/ignite-3/pull/2107
(no comment)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sk0x50 merged pull request #2107: IGNITE-19534 Fix error code duplication in the error message
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 merged PR #2107:
URL: https://github.com/apache/ignite-3/pull/2107
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sk0x50 commented on a diff in pull request #2107: IGNITE-19534 Fixed error code duplication in the error message
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #2107:
URL: https://github.com/apache/ignite-3/pull/2107#discussion_r1212710214
##########
modules/api/src/test/java/org/apache/ignite/lang/IgniteExceptionTest.java:
##########
@@ -32,34 +32,89 @@
*/
public class IgniteExceptionTest {
@Test
- public void testWrapUncheckedException() {
- var originalEx = new CustomTestException(UUID.randomUUID(), Table.TABLE_NOT_FOUND_ERR, "Error foo bar", null);
+ public void testWrapPublicUncheckedException() {
+ var originalMessage = "Error foo bar";
+ var originalTraceId = UUID.randomUUID();
+ var expectedFullMessage = CustomTestException.class.getName() + ": "
+ + errorMessage(originalTraceId, Table.TABLE_NOT_FOUND_ERR, originalMessage);
+
+ var originalEx = new CustomTestException(originalTraceId, Table.TABLE_NOT_FOUND_ERR, originalMessage, null);
var wrappedEx = new CompletionException(originalEx);
var res = IgniteException.wrap(wrappedEx);
- assertThat(res.getMessage(), containsString("Error foo bar"));
assertEquals(originalEx.traceId(), res.traceId());
assertEquals(originalEx.code(), res.code());
assertEquals(originalEx.getClass(), res.getClass());
assertSame(originalEx, res.getCause());
+ assertEquals(originalMessage, res.getMessage());
+ assertEquals(expectedFullMessage, res.toString());
}
@Test
- public void testWrapCheckedException() {
- var originalEx = new IgniteCheckedException(Table.COLUMN_ALREADY_EXISTS_ERR, "Msg.");
+ public void testWrapPublicCheckedException() {
+ var originalMessage = "Msg";
+ var originalTraceId = UUID.randomUUID();
+ var expectedFullMessage = IgniteException.class.getName() + ": "
+ + errorMessage(originalTraceId, Table.COLUMN_ALREADY_EXISTS_ERR, originalMessage);
+
+ var originalEx = new IgniteCheckedException(originalTraceId, Table.COLUMN_ALREADY_EXISTS_ERR, originalMessage);
var wrappedEx = new CompletionException(originalEx);
var res = IgniteException.wrap(wrappedEx);
- assertThat(res.getMessage(), containsString("Msg."));
assertEquals(originalEx.traceId(), res.traceId());
assertEquals(originalEx.code(), res.code());
assertSame(originalEx, res.getCause());
+ assertEquals(originalMessage, res.getMessage());
+ assertEquals(expectedFullMessage, res.toString());
+ }
+
+ @Test
+ public void testWrapInternalException() {
+ var originalMessage = "Unexpected error.";
+ var originalTraceId = UUID.randomUUID();
+
+ var originalEx = new IgniteInternalException(originalTraceId, Common.INTERNAL_ERR, originalMessage);
+ var wrappedEx = new CompletionException(originalEx);
+ var res = IgniteException.wrap(wrappedEx);
Review Comment:
Ok. this makes sense to me. I will 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sk0x50 commented on a diff in pull request #2107: IGNITE-19534 Fix error code duplication in the error message
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #2107:
URL: https://github.com/apache/ignite-3/pull/2107#discussion_r1212775945
##########
modules/api/src/test/java/org/apache/ignite/lang/IgniteExceptionTest.java:
##########
@@ -32,34 +32,89 @@
*/
public class IgniteExceptionTest {
@Test
- public void testWrapUncheckedException() {
- var originalEx = new CustomTestException(UUID.randomUUID(), Table.TABLE_NOT_FOUND_ERR, "Error foo bar", null);
+ public void testWrapPublicUncheckedException() {
+ var originalMessage = "Error foo bar";
+ var originalTraceId = UUID.randomUUID();
+ var expectedFullMessage = CustomTestException.class.getName() + ": "
+ + errorMessage(originalTraceId, Table.TABLE_NOT_FOUND_ERR, originalMessage);
+
+ var originalEx = new CustomTestException(originalTraceId, Table.TABLE_NOT_FOUND_ERR, originalMessage, null);
var wrappedEx = new CompletionException(originalEx);
var res = IgniteException.wrap(wrappedEx);
- assertThat(res.getMessage(), containsString("Error foo bar"));
assertEquals(originalEx.traceId(), res.traceId());
assertEquals(originalEx.code(), res.code());
assertEquals(originalEx.getClass(), res.getClass());
assertSame(originalEx, res.getCause());
+ assertEquals(originalMessage, res.getMessage());
+ assertEquals(expectedFullMessage, res.toString());
}
@Test
- public void testWrapCheckedException() {
- var originalEx = new IgniteCheckedException(Table.COLUMN_ALREADY_EXISTS_ERR, "Msg.");
+ public void testWrapPublicCheckedException() {
+ var originalMessage = "Msg";
+ var originalTraceId = UUID.randomUUID();
+ var expectedFullMessage = IgniteException.class.getName() + ": "
+ + errorMessage(originalTraceId, Table.COLUMN_ALREADY_EXISTS_ERR, originalMessage);
+
+ var originalEx = new IgniteCheckedException(originalTraceId, Table.COLUMN_ALREADY_EXISTS_ERR, originalMessage);
var wrappedEx = new CompletionException(originalEx);
var res = IgniteException.wrap(wrappedEx);
- assertThat(res.getMessage(), containsString("Msg."));
assertEquals(originalEx.traceId(), res.traceId());
assertEquals(originalEx.code(), res.code());
assertSame(originalEx, res.getCause());
+ assertEquals(originalMessage, res.getMessage());
+ assertEquals(expectedFullMessage, res.toString());
+ }
+
+ @Test
+ public void testWrapInternalException() {
+ var originalMessage = "Unexpected error.";
+ var originalTraceId = UUID.randomUUID();
+
+ var originalEx = new IgniteInternalException(originalTraceId, Common.INTERNAL_ERR, originalMessage);
+ var wrappedEx = new CompletionException(originalEx);
+ var res = IgniteException.wrap(wrappedEx);
Review Comment:
As was discussed in private, we decided to stick with the current code without any changes for this particular case
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2107: IGNITE-19534 Fixed error code duplication in the error message
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2107:
URL: https://github.com/apache/ignite-3/pull/2107#discussion_r1212075525
##########
modules/api/src/main/java/org/apache/ignite/lang/IgniteException.java:
##########
@@ -255,6 +252,14 @@ public UUID traceId() {
return traceId;
}
+ /** {@inheritDoc} */
+ @Override
+ public String toString() {
Review Comment:
It looks a bit weird that we had to duplicate exact the same code in all our ignite specific root exceptions toString() and similar, however I don't have better solution here.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2107: IGNITE-19534 Fixed error code duplication in the error message
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2107:
URL: https://github.com/apache/ignite-3/pull/2107#discussion_r1212059946
##########
modules/api/src/test/java/org/apache/ignite/lang/IgniteExceptionTest.java:
##########
@@ -32,34 +32,89 @@
*/
public class IgniteExceptionTest {
@Test
- public void testWrapUncheckedException() {
- var originalEx = new CustomTestException(UUID.randomUUID(), Table.TABLE_NOT_FOUND_ERR, "Error foo bar", null);
+ public void testWrapPublicUncheckedException() {
+ var originalMessage = "Error foo bar";
+ var originalTraceId = UUID.randomUUID();
+ var expectedFullMessage = CustomTestException.class.getName() + ": "
+ + errorMessage(originalTraceId, Table.TABLE_NOT_FOUND_ERR, originalMessage);
+
+ var originalEx = new CustomTestException(originalTraceId, Table.TABLE_NOT_FOUND_ERR, originalMessage, null);
var wrappedEx = new CompletionException(originalEx);
var res = IgniteException.wrap(wrappedEx);
- assertThat(res.getMessage(), containsString("Error foo bar"));
assertEquals(originalEx.traceId(), res.traceId());
assertEquals(originalEx.code(), res.code());
assertEquals(originalEx.getClass(), res.getClass());
assertSame(originalEx, res.getCause());
+ assertEquals(originalMessage, res.getMessage());
+ assertEquals(expectedFullMessage, res.toString());
}
@Test
- public void testWrapCheckedException() {
- var originalEx = new IgniteCheckedException(Table.COLUMN_ALREADY_EXISTS_ERR, "Msg.");
+ public void testWrapPublicCheckedException() {
+ var originalMessage = "Msg";
+ var originalTraceId = UUID.randomUUID();
+ var expectedFullMessage = IgniteException.class.getName() + ": "
+ + errorMessage(originalTraceId, Table.COLUMN_ALREADY_EXISTS_ERR, originalMessage);
+
+ var originalEx = new IgniteCheckedException(originalTraceId, Table.COLUMN_ALREADY_EXISTS_ERR, originalMessage);
var wrappedEx = new CompletionException(originalEx);
var res = IgniteException.wrap(wrappedEx);
- assertThat(res.getMessage(), containsString("Msg."));
assertEquals(originalEx.traceId(), res.traceId());
assertEquals(originalEx.code(), res.code());
assertSame(originalEx, res.getCause());
+ assertEquals(originalMessage, res.getMessage());
+ assertEquals(expectedFullMessage, res.toString());
+ }
+
+ @Test
+ public void testWrapInternalException() {
+ var originalMessage = "Unexpected error.";
+ var originalTraceId = UUID.randomUUID();
+
+ var originalEx = new IgniteInternalException(originalTraceId, Common.INTERNAL_ERR, originalMessage);
+ var wrappedEx = new CompletionException(originalEx);
+ var res = IgniteException.wrap(wrappedEx);
Review Comment:
Not sure, but formally speaking I believe it's not valid to use var in such cases. However, from my point of view it's still fine in given case.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sanpwc commented on a diff in pull request #2107: IGNITE-19534 Fixed error code duplication in the error message
Posted by "sanpwc (via GitHub)" <gi...@apache.org>.
sanpwc commented on code in PR #2107:
URL: https://github.com/apache/ignite-3/pull/2107#discussion_r1212060474
##########
modules/client-handler/src/integrationTest/java/org/apache/ignite/client/handler/ItClientHandlerMetricsTest.java:
##########
@@ -147,7 +147,7 @@ void testBytesSentReceived(TestInfo testInfo) throws Exception {
ItClientHandlerTestUtils.connectAndHandshake(serverModule, false, true);
assertTrue(
- IgniteTestUtils.waitForCondition(() -> testServer.metrics().bytesSent() == 216, 1000),
+ IgniteTestUtils.waitForCondition(() -> testServer.metrics().bytesSent() == 158, 1000),
Review Comment:
What's that?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sk0x50 commented on a diff in pull request #2107: IGNITE-19534 Fix error code duplication in the error message
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #2107:
URL: https://github.com/apache/ignite-3/pull/2107#discussion_r1212724055
##########
modules/api/src/main/java/org/apache/ignite/lang/IgniteException.java:
##########
@@ -255,6 +252,14 @@ public UUID traceId() {
return traceId;
}
+ /** {@inheritDoc} */
+ @Override
+ public String toString() {
Review Comment:
we are on the same page. :)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sk0x50 commented on a diff in pull request #2107: IGNITE-19534 Fix error code duplication in the error message
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #2107:
URL: https://github.com/apache/ignite-3/pull/2107#discussion_r1212720169
##########
modules/client-handler/src/integrationTest/java/org/apache/ignite/client/handler/ItClientHandlerMetricsTest.java:
##########
@@ -147,7 +147,7 @@ void testBytesSentReceived(TestInfo testInfo) throws Exception {
ItClientHandlerTestUtils.connectAndHandshake(serverModule, false, true);
assertTrue(
- IgniteTestUtils.waitForCondition(() -> testServer.metrics().bytesSent() == 216, 1000),
+ IgniteTestUtils.waitForCondition(() -> testServer.metrics().bytesSent() == 158, 1000),
Review Comment:
This particular line of code checks the size of an exception transferred from the server node. After the changes proposed in this PR, the size (in bytes) was reduced just because the exception message is not extended by the error code and trace identifier.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org