Re: [PR] IGNITE-25515 Remove obsolete code from MessageReader/Writer [ignite]
timoninmaxim merged PR #12110: URL: https://github.com/apache/ignite/pull/12110 -- 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-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] IGNITE-25515 Remove obsolete code from MessageReader/Writer [ignite]
sergey-chugunov-1985 commented on code in PR #12110: URL: https://github.com/apache/ignite/pull/12110#discussion_r2122959096 ## modules/core/src/main/java/org/apache/ignite/internal/processors/cache/version/GridCacheRawVersionedEntry.java: ## @@ -308,17 +305,11 @@ public void prepareDirectMarshal(CacheObjectContext ctx) throws IgniteCheckedExc } -assert key != null; Review Comment: > which information do you mean? My point here is that I want to describe explicitly the fact that message itself knows nothing about semantics of its fields and what are possible values for the fields. So enforsing rules like "when this field is null that field should be positive" is not a responsibility of a message itself but of a code that uses (consumes) the message. -- 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-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] IGNITE-25515 Remove obsolete code from MessageReader/Writer [ignite]
sergey-chugunov-1985 commented on code in PR #12110: URL: https://github.com/apache/ignite/pull/12110#discussion_r2122959096 ## modules/core/src/main/java/org/apache/ignite/internal/processors/cache/version/GridCacheRawVersionedEntry.java: ## @@ -308,17 +305,11 @@ public void prepareDirectMarshal(CacheObjectContext ctx) throws IgniteCheckedExc } -assert key != null; Review Comment: My point here is that I want to describe explicitly the fact that message itself knows nothing about semantics of its fields and what are possible values for the fields. So enforsing rules like "when this field is null that field should be positive" is not a responsibility of a message itself but of a code that uses (consumes) the message. -- 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-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] IGNITE-25515 Remove obsolete code from MessageReader/Writer [ignite]
timoninmaxim commented on code in PR #12110:
URL: https://github.com/apache/ignite/pull/12110#discussion_r2121605624
##
modules/core/src/main/java/org/apache/ignite/plugin/extensions/communication/MessageReader.java:
##
@@ -39,246 +39,198 @@ public interface MessageReader {
*/
public void setBuffer(ByteBuffer buf);
-/**
- * Sets type of message currently read.
- *
- * @param msgCls Message type.
- */
-public void setCurrentReadClass(Class msgCls);
-
-/**
- * Callback that must be invoked by a message implementation before
message body started decoding.
- *
- * @return {@code True} if reading can proceed, {@code false} otherwise.
- */
-public boolean beforeMessageRead();
-
-/**
- * Callback that must be invoked by a message implementation after message
body finished decoding.
- *
- * @param msgCls Message class finishing read stage.
- * @return {@code True} if reading can proceed, {@code false} otherwise.
- */
-public boolean afterMessageRead(Class msgCls);
-
/**
* Reads {@code byte} value.
*
- * @param name Field name.
* @return {@code byte} value.
*/
-public byte readByte(String name);
+public byte readByte();
/**
* Reads {@code short} value.
*
- * @param name Field name.
* @return {@code short} value.
*/
-public short readShort(String name);
+public short readShort();
/**
* Reads {@code int} value.
*
- * @param name Field name.
* @return {@code int} value.
*/
-public int readInt(String name);
+public int readInt();
/**
* Reads {@code int} value.
*
- * @param name Field name.
* @param dflt Default value if field not found.
* @return {@code int} value.
*/
-public int readInt(String name, int dflt);
+public int readInt(int dflt);
Review Comment:
I'll remove this method.
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-25515 Remove obsolete code from MessageReader/Writer [ignite]
timoninmaxim commented on code in PR #12110:
URL: https://github.com/apache/ignite/pull/12110#discussion_r2121601277
##
modules/core/src/test/java/org/apache/ignite/internal/managers/communication/IgniteIoCommunicationMessageSerializationTest.java:
##
@@ -59,33 +53,23 @@ private static class TestIoMessageReader extends
AbstractTestMessageReader {
/** */
private static final byte[] BYTE_ARR = toBytes(null);
-/** */
-protected Class msgCls;
-
/** */
public TestIoMessageReader(int capacity) {
super(capacity);
}
/** {@inheritDoc} */
-@Override public void setCurrentReadClass(Class
msgCls) {
-this.msgCls = msgCls;
-}
-
-/** {@inheritDoc} */
-@Override public byte[] readByteArray(String name) {
-super.readByteArray(name);
+@Override public byte[] readByteArray() {
+super.readByteArray();
return BYTE_ARR;
}
-/** {@inheritDoc} */
-@Override public T readMessage(String name) {
-super.readMessage(name);
-
-return msgCls.equals(GridCacheRawVersionedEntry.class) &&
"key".equals(name)
-? (T)new KeyCacheObjectImpl()
-: null;
-}
+///** {@inheritDoc} */
Review Comment:
I'll remove this code at all.
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-25515 Remove obsolete code from MessageReader/Writer [ignite]
sonarqubecloud[bot] commented on PR #12110: URL: https://github.com/apache/ignite/pull/12110#issuecomment-2931563558 ## [](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12110) **Quality Gate failed** Failed conditions  [26.2% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=12110&metric=new_duplicated_lines_density&view=list) (required ≤ 5%)  [558 Duplicated Blocks on New Code](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12110) (required ≤ 10)  [4 New Code Smells](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12110) (required ≤ 1) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12110) ##  Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request) -- 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-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] IGNITE-25515 Remove obsolete code from MessageReader/Writer [ignite]
timoninmaxim commented on code in PR #12110: URL: https://github.com/apache/ignite/pull/12110#discussion_r2121608570 ## modules/core/src/main/java/org/apache/ignite/internal/processors/cache/version/GridCacheRawVersionedEntry.java: ## @@ -308,17 +305,11 @@ public void prepareDirectMarshal(CacheObjectContext ctx) throws IgniteCheckedExc } -assert key != null; Review Comment: > Do I undestand correctly that we want to make outside code responsible for this sort of checks? Yes. > but we may want to make this information explicit which information do you mean? -- 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-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] IGNITE-25515 Remove obsolete code from MessageReader/Writer [ignite]
sergey-chugunov-1985 commented on code in PR #12110:
URL: https://github.com/apache/ignite/pull/12110#discussion_r2121364410
##
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/version/GridCacheRawVersionedEntry.java:
##
@@ -308,17 +305,11 @@ public void prepareDirectMarshal(CacheObjectContext ctx)
throws IgniteCheckedExc
}
-assert key != null;
Review Comment:
Do I undestand correctly that we want to make outside code responsible for
this sort of checks? From codegeneration standpoint of view this code doesn't
make sense, but we may want to make this information explicit - in javadoc of
Message interface or somewhere else.
##
modules/core/src/main/java/org/apache/ignite/plugin/extensions/communication/MessageReader.java:
##
@@ -39,246 +39,198 @@ public interface MessageReader {
*/
public void setBuffer(ByteBuffer buf);
-/**
- * Sets type of message currently read.
- *
- * @param msgCls Message type.
- */
-public void setCurrentReadClass(Class msgCls);
-
-/**
- * Callback that must be invoked by a message implementation before
message body started decoding.
- *
- * @return {@code True} if reading can proceed, {@code false} otherwise.
- */
-public boolean beforeMessageRead();
-
-/**
- * Callback that must be invoked by a message implementation after message
body finished decoding.
- *
- * @param msgCls Message class finishing read stage.
- * @return {@code True} if reading can proceed, {@code false} otherwise.
- */
-public boolean afterMessageRead(Class msgCls);
-
/**
* Reads {@code byte} value.
*
- * @param name Field name.
* @return {@code byte} value.
*/
-public byte readByte(String name);
+public byte readByte();
/**
* Reads {@code short} value.
*
- * @param name Field name.
* @return {@code short} value.
*/
-public short readShort(String name);
+public short readShort();
/**
* Reads {@code int} value.
*
- * @param name Field name.
* @return {@code int} value.
*/
-public int readInt(String name);
+public int readInt();
/**
* Reads {@code int} value.
*
- * @param name Field name.
* @param dflt Default value if field not found.
* @return {@code int} value.
*/
-public int readInt(String name, int dflt);
+public int readInt(int dflt);
Review Comment:
I wonder if we can get rid of this method too as its implementation simply
delegates to `readInt` w/o parameters, effectively ignoring any value passed
into as `dflt`.
##
modules/core/src/test/java/org/apache/ignite/internal/managers/communication/IgniteIoCommunicationMessageSerializationTest.java:
##
@@ -59,33 +53,23 @@ private static class TestIoMessageReader extends
AbstractTestMessageReader {
/** */
private static final byte[] BYTE_ARR = toBytes(null);
-/** */
-protected Class msgCls;
-
/** */
public TestIoMessageReader(int capacity) {
super(capacity);
}
/** {@inheritDoc} */
-@Override public void setCurrentReadClass(Class
msgCls) {
-this.msgCls = msgCls;
-}
-
-/** {@inheritDoc} */
-@Override public byte[] readByteArray(String name) {
-super.readByteArray(name);
+@Override public byte[] readByteArray() {
+super.readByteArray();
return BYTE_ARR;
}
-/** {@inheritDoc} */
-@Override public T readMessage(String name) {
-super.readMessage(name);
-
-return msgCls.equals(GridCacheRawVersionedEntry.class) &&
"key".equals(name)
-? (T)new KeyCacheObjectImpl()
-: null;
-}
+///** {@inheritDoc} */
Review Comment:
Could you please leave a comment here clarifying why this peice of code is
commented out and when it can be uncommented?
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-25515 Remove obsolete code from MessageReader/Writer [ignite]
sergey-chugunov-1985 commented on code in PR #12110:
URL: https://github.com/apache/ignite/pull/12110#discussion_r2121073257
##
modules/codegen/src/main/java/org/apache/ignite/codegen/MessageCodeGenerator.java:
##
@@ -683,7 +663,6 @@ else if (Map.class.isAssignableFrom(type)) {
/**
* @param type Field type.
- * @param name Field name.
* @param colItemType Collection item type.
Review Comment:
I don't think we should remove `@param name` documentation here, as field
name is still used by the method implementation.
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] IGNITE-25515 Remove obsolete code from MessageReader/Writer [ignite]
sonarqubecloud[bot] commented on PR #12110: URL: https://github.com/apache/ignite/pull/12110#issuecomment-2929685327 ## [](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12110) **Quality Gate failed** Failed conditions  [26.1% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=12110&metric=new_duplicated_lines_density&view=list) (required ≤ 5%)  [5 New Code Smells](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12110) (required ≤ 1)  [558 Duplicated Blocks on New Code](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12110) (required ≤ 10) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12110) ##  Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request) -- 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-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] IGNITE-25515 Remove obsolete code from MessageReader/Writer [ignite]
sonarqubecloud[bot] commented on PR #12110: URL: https://github.com/apache/ignite/pull/12110#issuecomment-2919354153 ## [](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12110) **Quality Gate failed** Failed conditions  [26.1% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_ignite&pullRequest=12110&metric=new_duplicated_lines_density&view=list) (required ≤ 5%)  [557 Duplicated Blocks on New Code](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12110) (required ≤ 10)  [4 New Code Smells](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12110) (required ≤ 1) [See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_ignite&pullRequest=12110) ##  Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request) -- 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-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
