[jira] [Commented] (IGNITE-12568) MessageFactory implementations refactoring
[
https://issues.apache.org/jira/browse/IGNITE-12568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17029930#comment-17029930
]
Andrey N. Gura commented on IGNITE-12568:
-
Merged to master branch.
> MessageFactory implementations refactoring
> --
>
> Key: IGNITE-12568
> URL: https://issues.apache.org/jira/browse/IGNITE-12568
> Project: Ignite
> Issue Type: Improvement
>Reporter: Andrey N. Gura
>Assignee: Andrey N. Gura
>Priority: Major
> Fix For: 2.9
>
> Time Spent: 1h 50m
> Remaining Estimate: 0h
>
> Currently existing {{MessageFactory}} implementations (at least
> {{GridIoMessageFactory}} and {{GridH2ValueMessageFactory}}) have serious
> problem: it is possible to register several messages with the same direct
> type. It could lead to the cluster freeze due to unexpected unmarshaling
> error.
> *Solution:*
> Each message should be registered and during registration we can check that
> there is no registered message with the same type. Otherwise we should not
> start node and print error message.
> *Proposed implementation:*
> Instead of {{switch-case}} block new array of size 2^16 should be created.
> Each array cell should contain message default constructor reference or
> lambda which is responsible for message instance creation. So during message
> unmarshaling system just lookup ctor-ref or lambda from array by index and
> invoke it in order to create proper message instance.
> All message factory extensions and custom message should be registered at the
> same array before communication SPI will be started.
> If we try to register message with direct type for which already exists
> registered message then node start process must be terminated (directly, with
> out any failure handlers).
> If we get message with unknown direct type (there is now registered message
> for this direct type) then failure handler be invoked but this is topic for
> discussion and should be considered separately.
> It could affect perfromance so we should run microbenchmark for this change.
> Additional benefit of this change is improving code readability. At present
> we have the following code:
> {code:java}
> @Override public Message create(short type) {
> Message msg = null;
> switch (type) {
> case -61:
> msg = new IgniteDiagnosticMessage();
> break;
> case -53:
> msg = new SchemaOperationStatusMessage();
> break;
> // etc.
> }
> {code}
> After refactoring it will looks like:
> {code:java}
> private Supplier[] msgCtrs;
> public GridIoMessageFactory(MessageFactory[] ext) {
> this.ext = ext;
>registerMessage(-61, IgniteDiagnosticMessage::new)
>registerMessage(-53, SchemaOperationStatusMessage::new)
> }
> @Override public Message create(short type) {
> return msgCtros[type].get();
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (IGNITE-12568) MessageFactory implementations refactoring
[
https://issues.apache.org/jira/browse/IGNITE-12568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17029918#comment-17029918
]
Alexey Goncharuk commented on IGNITE-12568:
---
Looks good to me as well!
> MessageFactory implementations refactoring
> --
>
> Key: IGNITE-12568
> URL: https://issues.apache.org/jira/browse/IGNITE-12568
> Project: Ignite
> Issue Type: Improvement
>Reporter: Andrey N. Gura
>Assignee: Andrey N. Gura
>Priority: Major
> Fix For: 2.9
>
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
> Currently existing {{MessageFactory}} implementations (at least
> {{GridIoMessageFactory}} and {{GridH2ValueMessageFactory}}) have serious
> problem: it is possible to register several messages with the same direct
> type. It could lead to the cluster freeze due to unexpected unmarshaling
> error.
> *Solution:*
> Each message should be registered and during registration we can check that
> there is no registered message with the same type. Otherwise we should not
> start node and print error message.
> *Proposed implementation:*
> Instead of {{switch-case}} block new array of size 2^16 should be created.
> Each array cell should contain message default constructor reference or
> lambda which is responsible for message instance creation. So during message
> unmarshaling system just lookup ctor-ref or lambda from array by index and
> invoke it in order to create proper message instance.
> All message factory extensions and custom message should be registered at the
> same array before communication SPI will be started.
> If we try to register message with direct type for which already exists
> registered message then node start process must be terminated (directly, with
> out any failure handlers).
> If we get message with unknown direct type (there is now registered message
> for this direct type) then failure handler be invoked but this is topic for
> discussion and should be considered separately.
> It could affect perfromance so we should run microbenchmark for this change.
> Additional benefit of this change is improving code readability. At present
> we have the following code:
> {code:java}
> @Override public Message create(short type) {
> Message msg = null;
> switch (type) {
> case -61:
> msg = new IgniteDiagnosticMessage();
> break;
> case -53:
> msg = new SchemaOperationStatusMessage();
> break;
> // etc.
> }
> {code}
> After refactoring it will looks like:
> {code:java}
> private Supplier[] msgCtrs;
> public GridIoMessageFactory(MessageFactory[] ext) {
> this.ext = ext;
>registerMessage(-61, IgniteDiagnosticMessage::new)
>registerMessage(-53, SchemaOperationStatusMessage::new)
> }
> @Override public Message create(short type) {
> return msgCtros[type].get();
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (IGNITE-12568) MessageFactory implementations refactoring
[
https://issues.apache.org/jira/browse/IGNITE-12568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17029851#comment-17029851
]
Ivan Bessonov commented on IGNITE-12568:
[~agura] looks good to me, do we need another approval from [~agoncharuk]?
> MessageFactory implementations refactoring
> --
>
> Key: IGNITE-12568
> URL: https://issues.apache.org/jira/browse/IGNITE-12568
> Project: Ignite
> Issue Type: Improvement
>Reporter: Andrey N. Gura
>Assignee: Andrey N. Gura
>Priority: Major
> Fix For: 2.9
>
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
> Currently existing {{MessageFactory}} implementations (at least
> {{GridIoMessageFactory}} and {{GridH2ValueMessageFactory}}) have serious
> problem: it is possible to register several messages with the same direct
> type. It could lead to the cluster freeze due to unexpected unmarshaling
> error.
> *Solution:*
> Each message should be registered and during registration we can check that
> there is no registered message with the same type. Otherwise we should not
> start node and print error message.
> *Proposed implementation:*
> Instead of {{switch-case}} block new array of size 2^16 should be created.
> Each array cell should contain message default constructor reference or
> lambda which is responsible for message instance creation. So during message
> unmarshaling system just lookup ctor-ref or lambda from array by index and
> invoke it in order to create proper message instance.
> All message factory extensions and custom message should be registered at the
> same array before communication SPI will be started.
> If we try to register message with direct type for which already exists
> registered message then node start process must be terminated (directly, with
> out any failure handlers).
> If we get message with unknown direct type (there is now registered message
> for this direct type) then failure handler be invoked but this is topic for
> discussion and should be considered separately.
> It could affect perfromance so we should run microbenchmark for this change.
> Additional benefit of this change is improving code readability. At present
> we have the following code:
> {code:java}
> @Override public Message create(short type) {
> Message msg = null;
> switch (type) {
> case -61:
> msg = new IgniteDiagnosticMessage();
> break;
> case -53:
> msg = new SchemaOperationStatusMessage();
> break;
> // etc.
> }
> {code}
> After refactoring it will looks like:
> {code:java}
> private Supplier[] msgCtrs;
> public GridIoMessageFactory(MessageFactory[] ext) {
> this.ext = ext;
>registerMessage(-61, IgniteDiagnosticMessage::new)
>registerMessage(-53, SchemaOperationStatusMessage::new)
> }
> @Override public Message create(short type) {
> return msgCtros[type].get();
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (IGNITE-12568) MessageFactory implementations refactoring
[
https://issues.apache.org/jira/browse/IGNITE-12568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17029723#comment-17029723
]
Andrey N. Gura commented on IGNITE-12568:
-
[~ibessonov] Thanks for your comments. I fixed most of them and replied on
other ones. Please, take a look.
> MessageFactory implementations refactoring
> --
>
> Key: IGNITE-12568
> URL: https://issues.apache.org/jira/browse/IGNITE-12568
> Project: Ignite
> Issue Type: Improvement
>Reporter: Andrey N. Gura
>Assignee: Andrey N. Gura
>Priority: Major
> Fix For: 2.9
>
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
> Currently existing {{MessageFactory}} implementations (at least
> {{GridIoMessageFactory}} and {{GridH2ValueMessageFactory}}) have serious
> problem: it is possible to register several messages with the same direct
> type. It could lead to the cluster freeze due to unexpected unmarshaling
> error.
> *Solution:*
> Each message should be registered and during registration we can check that
> there is no registered message with the same type. Otherwise we should not
> start node and print error message.
> *Proposed implementation:*
> Instead of {{switch-case}} block new array of size 2^16 should be created.
> Each array cell should contain message default constructor reference or
> lambda which is responsible for message instance creation. So during message
> unmarshaling system just lookup ctor-ref or lambda from array by index and
> invoke it in order to create proper message instance.
> All message factory extensions and custom message should be registered at the
> same array before communication SPI will be started.
> If we try to register message with direct type for which already exists
> registered message then node start process must be terminated (directly, with
> out any failure handlers).
> If we get message with unknown direct type (there is now registered message
> for this direct type) then failure handler be invoked but this is topic for
> discussion and should be considered separately.
> It could affect perfromance so we should run microbenchmark for this change.
> Additional benefit of this change is improving code readability. At present
> we have the following code:
> {code:java}
> @Override public Message create(short type) {
> Message msg = null;
> switch (type) {
> case -61:
> msg = new IgniteDiagnosticMessage();
> break;
> case -53:
> msg = new SchemaOperationStatusMessage();
> break;
> // etc.
> }
> {code}
> After refactoring it will looks like:
> {code:java}
> private Supplier[] msgCtrs;
> public GridIoMessageFactory(MessageFactory[] ext) {
> this.ext = ext;
>registerMessage(-61, IgniteDiagnosticMessage::new)
>registerMessage(-53, SchemaOperationStatusMessage::new)
> }
> @Override public Message create(short type) {
> return msgCtros[type].get();
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (IGNITE-12568) MessageFactory implementations refactoring
[
https://issues.apache.org/jira/browse/IGNITE-12568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17027436#comment-17027436
]
Ignite TC Bot commented on IGNITE-12568:
{panel:title=Branch: [pull/7302/head] Base: [master] : No blockers
found!|borderStyle=dashed|borderColor=#ccc|titleBGColor=#D6F7C1}{panel}
[TeamCity *-- Run :: All*
Results|https://ci.ignite.apache.org/viewLog.html?buildId=4972045buildTypeId=IgniteTests24Java8_RunAll]
> MessageFactory implementations refactoring
> --
>
> Key: IGNITE-12568
> URL: https://issues.apache.org/jira/browse/IGNITE-12568
> Project: Ignite
> Issue Type: Improvement
>Reporter: Andrey N. Gura
>Assignee: Andrey N. Gura
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Currently existing {{MessageFactory}} implementations (at least
> {{GridIoMessageFactory}} and {{GridH2ValueMessageFactory}}) have serious
> problem: it is possible to register several messages with the same direct
> type. It could lead to the cluster freeze due to unexpected unmarshaling
> error.
> *Solution:*
> Each message should be registered and during registration we can check that
> there is no registered message with the same type. Otherwise we should not
> start node and print error message.
> *Proposed implementation:*
> Instead of {{switch-case}} block new array of size 2^16 should be created.
> Each array cell should contain message default constructor reference or
> lambda which is responsible for message instance creation. So during message
> unmarshaling system just lookup ctor-ref or lambda from array by index and
> invoke it in order to create proper message instance.
> All message factory extensions and custom message should be registered at the
> same array before communication SPI will be started.
> If we try to register message with direct type for which already exists
> registered message then node start process must be terminated (directly, with
> out any failure handlers).
> If we get message with unknown direct type (there is now registered message
> for this direct type) then failure handler be invoked.
> It could affect perfromance so we should run microbenchmark for this change.
> Additional benefit of this change is improving code readability. At present
> we have the following code:
> {code:java}
> @Override public Message create(short type) {
> Message msg = null;
> switch (type) {
> case -61:
> msg = new IgniteDiagnosticMessage();
> break;
> case -53:
> msg = new SchemaOperationStatusMessage();
> break;
> // etc.
> }
> {code}
> After refactoring it will looks like:
> {code:java}
> private Supplier[] msgCtrs;
> public GridIoMessageFactory(MessageFactory[] ext) {
> this.ext = ext;
>registerMessage(-61, IgniteDiagnosticMessage::new)
>registerMessage(-53, SchemaOperationStatusMessage::new)
> }
> @Override public Message create(short type) {
> return msgCtros[type].get();
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (IGNITE-12568) MessageFactory implementations refactoring
[
https://issues.apache.org/jira/browse/IGNITE-12568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17026033#comment-17026033
]
Andrey N. Gura commented on IGNITE-12568:
-
Added new test. Waiting for TC.
> MessageFactory implementations refactoring
> --
>
> Key: IGNITE-12568
> URL: https://issues.apache.org/jira/browse/IGNITE-12568
> Project: Ignite
> Issue Type: Improvement
>Reporter: Andrey N. Gura
>Assignee: Andrey N. Gura
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Currently existing {{MessageFactory}} implementations (at least
> {{GridIoMessageFactory}} and {{GridH2ValueMessageFactory}}) have serious
> problem: it is possible to register several messages with the same direct
> type. It could lead to the cluster freeze due to unexpected unmarshaling
> error.
> *Solution:*
> Each message should be registered and during registration we can check that
> there is no registered message with the same type. Otherwise we should not
> start node and print error message.
> *Proposed implementation:*
> Instead of {{switch-case}} block new array of size 2^16 should be created.
> Each array cell should contain message default constructor reference or
> lambda which is responsible for message instance creation. So during message
> unmarshaling system just lookup ctor-ref or lambda from array by index and
> invoke it in order to create proper message instance.
> All message factory extensions and custom message should be registered at the
> same array before communication SPI will be started.
> If we try to register message with direct type for which already exists
> registered message then node start process must be terminated (directly, with
> out any failure handlers).
> If we get message with unknown direct type (there is now registered message
> for this direct type) then failure handler be invoked.
> It could affect perfromance so we should run microbenchmark for this change.
> Additional benefit of this change is improving code readability. At present
> we have the following code:
> {code:java}
> @Override public Message create(short type) {
> Message msg = null;
> switch (type) {
> case -61:
> msg = new IgniteDiagnosticMessage();
> break;
> case -53:
> msg = new SchemaOperationStatusMessage();
> break;
> // etc.
> }
> {code}
> After refactoring it will looks like:
> {code:java}
> private Supplier[] msgCtrs;
> public GridIoMessageFactory(MessageFactory[] ext) {
> this.ext = ext;
>registerMessage(-61, IgniteDiagnosticMessage::new)
>registerMessage(-53, SchemaOperationStatusMessage::new)
> }
> @Override public Message create(short type) {
> return msgCtros[type].get();
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (IGNITE-12568) MessageFactory implementations refactoring
[
https://issues.apache.org/jira/browse/IGNITE-12568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17025141#comment-17025141
]
Andrey N. Gura commented on IGNITE-12568:
-
[~ibessonov] [~agoncharuk] Could you please review change?
> MessageFactory implementations refactoring
> --
>
> Key: IGNITE-12568
> URL: https://issues.apache.org/jira/browse/IGNITE-12568
> Project: Ignite
> Issue Type: Improvement
>Reporter: Andrey N. Gura
>Assignee: Andrey N. Gura
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Currently existing {{MessageFactory}} implementations (at least
> {{GridIoMessageFactory}} and {{GridH2ValueMessageFactory}}) have serious
> problem: it is possible to register several messages with the same direct
> type. It could lead to the cluster freeze due to unexpected unmarshaling
> error.
> *Solution:*
> Each message should be registered and during registration we can check that
> there is no registered message with the same type. Otherwise we should not
> start node and print error message.
> *Proposed implementation:*
> Instead of {{switch-case}} block new array of size 2^16 should be created.
> Each array cell should contain message default constructor reference or
> lambda which is responsible for message instance creation. So during message
> unmarshaling system just lookup ctor-ref or lambda from array by index and
> invoke it in order to create proper message instance.
> All message factory extensions and custom message should be registered at the
> same array before communication SPI will be started.
> If we try to register message with direct type for which already exists
> registered message then node start process must be terminated (directly, with
> out any failure handlers).
> If we get message with unknown direct type (there is now registered message
> for this direct type) then failure handler be invoked.
> It could affect perfromance so we should run microbenchmark for this change.
> Additional benefit of this change is improving code readability. At present
> we have the following code:
> {code:java}
> @Override public Message create(short type) {
> Message msg = null;
> switch (type) {
> case -61:
> msg = new IgniteDiagnosticMessage();
> break;
> case -53:
> msg = new SchemaOperationStatusMessage();
> break;
> // etc.
> }
> {code}
> After refactoring it will looks like:
> {code:java}
> private Supplier[] msgCtrs;
> public GridIoMessageFactory(MessageFactory[] ext) {
> this.ext = ext;
>registerMessage(-61, IgniteDiagnosticMessage::new)
>registerMessage(-53, SchemaOperationStatusMessage::new)
> }
> @Override public Message create(short type) {
> return msgCtros[type].get();
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (IGNITE-12568) MessageFactory implementations refactoring
[
https://issues.apache.org/jira/browse/IGNITE-12568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17025139#comment-17025139
]
Ignite TC Bot commented on IGNITE-12568:
{panel:title=Branch: [pull/7302/head] Base: [master] : No blockers
found!|borderStyle=dashed|borderColor=#ccc|titleBGColor=#D6F7C1}{panel}
[TeamCity *-- Run :: All*
Results|https://ci.ignite.apache.org/viewLog.html?buildId=4964870buildTypeId=IgniteTests24Java8_RunAll]
> MessageFactory implementations refactoring
> --
>
> Key: IGNITE-12568
> URL: https://issues.apache.org/jira/browse/IGNITE-12568
> Project: Ignite
> Issue Type: Improvement
>Reporter: Andrey N. Gura
>Assignee: Andrey N. Gura
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Currently existing {{MessageFactory}} implementations (at least
> {{GridIoMessageFactory}} and {{GridH2ValueMessageFactory}}) have serious
> problem: it is possible to register several messages with the same direct
> type. It could lead to the cluster freeze due to unexpected unmarshaling
> error.
> *Solution:*
> Each message should be registered and during registration we can check that
> there is no registered message with the same type. Otherwise we should not
> start node and print error message.
> *Proposed implementation:*
> Instead of {{switch-case}} block new array of size 2^16 should be created.
> Each array cell should contain message default constructor reference or
> lambda which is responsible for message instance creation. So during message
> unmarshaling system just lookup ctor-ref or lambda from array by index and
> invoke it in order to create proper message instance.
> All message factory extensions and custom message should be registered at the
> same array before communication SPI will be started.
> If we try to register message with direct type for which already exists
> registered message then node start process must be terminated (directly, with
> out any failure handlers).
> If we get message with unknown direct type (there is now registered message
> for this direct type) then failure handler be invoked.
> It could affect perfromance so we should run microbenchmark for this change.
> Additional benefit of this change is improving code readability. At present
> we have the following code:
> {code:java}
> @Override public Message create(short type) {
> Message msg = null;
> switch (type) {
> case -61:
> msg = new IgniteDiagnosticMessage();
> break;
> case -53:
> msg = new SchemaOperationStatusMessage();
> break;
> // etc.
> }
> {code}
> After refactoring it will looks like:
> {code:java}
> private Supplier[] msgCtrs;
> public GridIoMessageFactory(MessageFactory[] ext) {
> this.ext = ext;
>registerMessage(-61, IgniteDiagnosticMessage::new)
>registerMessage(-53, SchemaOperationStatusMessage::new)
> }
> @Override public Message create(short type) {
> return msgCtros[type].get();
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (IGNITE-12568) MessageFactory implementations refactoring
[
https://issues.apache.org/jira/browse/IGNITE-12568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17023037#comment-17023037
]
Alexey Goncharuk commented on IGNITE-12568:
---
Agree, the second approach looks clean.
> MessageFactory implementations refactoring
> --
>
> Key: IGNITE-12568
> URL: https://issues.apache.org/jira/browse/IGNITE-12568
> Project: Ignite
> Issue Type: Improvement
>Reporter: Andrey N. Gura
>Assignee: Andrey N. Gura
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Currently existing {{MessageFactory}} implementations (at least
> {{GridIoMessageFactory}} and {{GridH2ValueMessageFactory}}) have serious
> problem: it is possible to register several messages with the same direct
> type. It could lead to the cluster freeze due to unexpected unmarshaling
> error.
> *Solution:*
> Each message should be registered and during registration we can check that
> there is no registered message with the same type. Otherwise we should not
> start node and print error message.
> *Proposed implementation:*
> Instead of {{switch-case}} block new array of size 2^16 should be created.
> Each array cell should contain message default constructor reference or
> lambda which is responsible for message instance creation. So during message
> unmarshaling system just lookup ctor-ref or lambda from array by index and
> invoke it in order to create proper message instance.
> All message factory extensions and custom message should be registered at the
> same array before communication SPI will be started.
> If we try to register message with direct type for which already exists
> registered message then node start process must be terminated (directly, with
> out any failure handlers).
> If we get message with unknown direct type (there is now registered message
> for this direct type) then failure handler be invoked.
> It could affect perfromance so we should run microbenchmark for this change.
> Additional benefit of this change is improving code readability. At present
> we have the following code:
> {code:java}
> @Override public Message create(short type) {
> Message msg = null;
> switch (type) {
> case -61:
> msg = new IgniteDiagnosticMessage();
> break;
> case -53:
> msg = new SchemaOperationStatusMessage();
> break;
> // etc.
> }
> {code}
> After refactoring it will looks like:
> {code:java}
> private Supplier[] msgCtrs;
> public GridIoMessageFactory(MessageFactory[] ext) {
> this.ext = ext;
>registerMessage(-61, IgniteDiagnosticMessage::new)
>registerMessage(-53, SchemaOperationStatusMessage::new)
> }
> @Override public Message create(short type) {
> return msgCtros[type].get();
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (IGNITE-12568) MessageFactory implementations refactoring
[
https://issues.apache.org/jira/browse/IGNITE-12568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17022058#comment-17022058
]
Ivan Bessonov commented on IGNITE-12568:
[~agura] second solution looks nicer, I like it, thank you!
> MessageFactory implementations refactoring
> --
>
> Key: IGNITE-12568
> URL: https://issues.apache.org/jira/browse/IGNITE-12568
> Project: Ignite
> Issue Type: Improvement
>Reporter: Andrey N. Gura
>Assignee: Andrey N. Gura
>Priority: Major
>
> Currently existing {{MessageFactory}} implementations (at least
> {{GridIoMessageFactory}} and {{GridH2ValueMessageFactory}}) have serious
> problem: it is possible to register several messages with the same direct
> type. It could lead to the cluster freeze due to unexpected unmarshaling
> error.
> *Solution:*
> Each message should be registered and during registration we can check that
> there is no registered message with the same type. Otherwise we should not
> start node and print error message.
> *Proposed implementation:*
> Instead of {{switch-case}} block new array of size 2^16 should be created.
> Each array cell should contain message default constructor reference or
> lambda which is responsible for message instance creation. So during message
> unmarshaling system just lookup ctor-ref or lambda from array by index and
> invoke it in order to create proper message instance.
> All message factory extensions and custom message should be registered at the
> same array before communication SPI will be started.
> If we try to register message with direct type for which already exists
> registered message then node start process must be terminated (directly, with
> out any failure handlers).
> If we get message with unknown direct type (there is now registered message
> for this direct type) then failure handler be invoked.
> It could affect perfromance so we should run microbenchmark for this change.
> Additional benefit of this change is improving code readability. At present
> we have the following code:
> {code:java}
> @Override public Message create(short type) {
> Message msg = null;
> switch (type) {
> case -61:
> msg = new IgniteDiagnosticMessage();
> break;
> case -53:
> msg = new SchemaOperationStatusMessage();
> break;
> // etc.
> }
> {code}
> After refactoring it will looks like:
> {code:java}
> private Supplier[] msgCtrs;
> public GridIoMessageFactory(MessageFactory[] ext) {
> this.ext = ext;
>registerMessage(-61, IgniteDiagnosticMessage::new)
>registerMessage(-53, SchemaOperationStatusMessage::new)
> }
> @Override public Message create(short type) {
> return msgCtros[type].get();
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (IGNITE-12568) MessageFactory implementations refactoring
[
https://issues.apache.org/jira/browse/IGNITE-12568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17022048#comment-17022048
]
Andrey N. Gura commented on IGNITE-12568:
-
[~ibessonov] [~agoncharuk] It seems that encapsulation is not problem if we'll
introduce some method that will return all message types and message suppliers.
{{MessageFactory}} interface is part of public API and it is a problem. So I
propose the following:
Introduce {{MessageFactoryEx}} interface as temp solution (should be changed in
Apache Ignite 3.0)
{code:java}
public interface MessageFactoryEx extends MessageFactory {
/**
* Returns set of all message suppliers with their direct types.
*/
Set>> messages();
}
{code}
{{GridIoMessageFactory}} will invoke {{messages()}} method for all extensions
if they implements {{MessageFactory}} interface.
Another solution requires two entities. {{IgniteMessageFactory}} class that is
aggregate for all message factories and {{MessageFactoryProvider}} that is
message suppliers provider:
{code:java}
public interface MessageFactoryProvider extends MessageFactory {
/**
* Register all message suppliers.
*/
void registerAll(IgniteMessageFactory factory);
}
{code}
{code:java}
/**
* Implementation of this interface
*/
public class IgniteMessageFactory implements MessageFactoryProvider {
private final Supplier[] msgSuppliers = new
Supplier[2^16];
// Here GridIoMessageFactory also will passed as exts.
public IgniteMessageFactory(MessageFactory[] exts) {
// Pseudocode
Set oldFactories = new HashSet();
// Iter over exts and register all messages form newly introsuced
providers
// if ext instaceof MessageFactoryProvider
// ext.registerAll(this);
// else
// oldFactories.add(ext);
// brute force
// iter over msgSuppliers array and for empty elements try to create
messages via oldFactories
// if oldFactory.create(type) != null
registerMesage(type, oldFactory::create)
}
/**
* Creates and returns messages instances of given type.
*/
public Message create(short type) {
// implementation
}
/**
* Registers supplier for given message type. This method should be called
by MessageFactoryEx implementation.
*/
public void registerMessage(short type, Supplier supplier) {
// implementation
}
public void registerAll(IgniteMessageFactory factory) {
registerMessage(1, SomeMessage::new)
...
}
}
{code}
This solution provides backward compatibility and allows detect messages with
the same direct type.
WDYT?
> MessageFactory implementations refactoring
> --
>
> Key: IGNITE-12568
> URL: https://issues.apache.org/jira/browse/IGNITE-12568
> Project: Ignite
> Issue Type: Improvement
>Reporter: Andrey N. Gura
>Assignee: Andrey N. Gura
>Priority: Major
>
> Currently existing {{MessageFactory}} implementations (at least
> {{GridIoMessageFactory}} and {{GridH2ValueMessageFactory}}) have serious
> problem: it is possible to register several messages with the same direct
> type. It could lead to the cluster freeze due to unexpected unmarshaling
> error.
> *Solution:*
> Each message should be registered and during registration we can check that
> there is no registered message with the same type. Otherwise we should not
> start node and print error message.
> *Proposed implementation:*
> Instead of {{switch-case}} block new array of size 2^16 should be created.
> Each array cell should contain message default constructor reference or
> lambda which is responsible for message instance creation. So during message
> unmarshaling system just lookup ctor-ref or lambda from array by index and
> invoke it in order to create proper message instance.
> All message factory extensions and custom message should be registered at the
> same array before communication SPI will be started.
> If we try to register message with direct type for which already exists
> registered message then node start process must be terminated (directly, with
> out any failure handlers).
> If we get message with unknown direct type (there is now registered message
> for this direct type) then failure handler be invoked.
> It could affect perfromance so we should run microbenchmark for this change.
> Additional benefit of this change is improving code readability. At present
> we have the following code:
> {code:java}
> @Override public Message create(short type) {
> Message msg = null;
> switch (type) {
> case -61:
> msg = new IgniteDiagnosticMessage();
> break;
> case -53:
> msg = new SchemaOperationStatusMessage();
[jira] [Commented] (IGNITE-12568) MessageFactory implementations refactoring
[
https://issues.apache.org/jira/browse/IGNITE-12568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17021255#comment-17021255
]
Andrey N. Gura commented on IGNITE-12568:
-
[~ibessonov] Good point about extensions. I need additional time to think about
it.
About performance... Also good idea. But we should measure all three solution
in order to choose the best of them.
> MessageFactory implementations refactoring
> --
>
> Key: IGNITE-12568
> URL: https://issues.apache.org/jira/browse/IGNITE-12568
> Project: Ignite
> Issue Type: Improvement
>Reporter: Andrey N. Gura
>Assignee: Andrey N. Gura
>Priority: Major
>
> Currently existing {{MessageFactory}} implementations (at least
> {{GridIoMessageFactory}} and {{GridH2ValueMessageFactory}}) have serious
> problem: it is possible to register several messages with the same direct
> type. It could lead to the cluster freeze due to unexpected unmarshaling
> error.
> *Solution:*
> Each message should be registered and during registration we can check that
> there is no registered message with the same type. Otherwise we should not
> start node and print error message.
> *Proposed implementation:*
> Instead of {{switch-case}} block new array of size 2^16 should be created.
> Each array cell should contain message default constructor reference or
> lambda which is responsible for message instance creation. So during message
> unmarshaling system just lookup ctor-ref or lambda from array by index and
> invoke it in order to create proper message instance.
> All message factory extensions and custom message should be registered at the
> same array before communication SPI will be started.
> If we try to register message with direct type for which already exists
> registered message then node start process must be terminated (directly, with
> out any failure handlers).
> If we get message with unknown direct type (there is now registered message
> for this direct type) then failure handler be invoked.
> It could affect perfromance so we should run microbenchmark for this change.
> Additional benefit of this change is improving code readability. At present
> we have the following code:
> {code:java}
> @Override public Message create(short type) {
> Message msg = null;
> switch (type) {
> case -61:
> msg = new IgniteDiagnosticMessage();
> break;
> case -53:
> msg = new SchemaOperationStatusMessage();
> break;
> // etc.
> }
> {code}
> After refactoring it will looks like:
> {code:java}
> private Supplier[] msgCtrs;
> public GridIoMessageFactory(MessageFactory[] ext) {
> this.ext = ext;
>registerMessage(-61, IgniteDiagnosticMessage::new)
>registerMessage(-53, SchemaOperationStatusMessage::new)
> }
> @Override public Message create(short type) {
> return msgCtros[type].get();
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Commented] (IGNITE-12568) MessageFactory implementations refactoring
[
https://issues.apache.org/jira/browse/IGNITE-12568?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17021208#comment-17021208
]
Ivan Bessonov commented on IGNITE-12568:
[~agura] I believe that we'll face an issue with encapsulation.
>From what I can tell, you mean "GridIoMessageFactory" here and we should
>inline extension message factories inside of it. But it's not clear how it can
>be done - those extension factories may not have the same array and there's no
>way of getting the required information from them rather then brute force. Is
>that acceptable?
Another concern - what will happen with performance? I can't predict anything,
but I propose 3rd way of creating messages - storing prototype objects for each
message type and cloning them when needed. Might be a good solution considering
that extension factories won't give us their lambdas to create messages. What
do you think?
> MessageFactory implementations refactoring
> --
>
> Key: IGNITE-12568
> URL: https://issues.apache.org/jira/browse/IGNITE-12568
> Project: Ignite
> Issue Type: Improvement
>Reporter: Andrey N. Gura
>Assignee: Andrey N. Gura
>Priority: Major
>
> Currently existing {{MessageFactory}} implementations (at least
> {{GridIoMessageFactory}} and {{GridH2ValueMessageFactory}}) have serious
> problem: it is possible to register several messages with the same direct
> type. It could lead to the cluster freeze due to unexpected unmarshaling
> error.
> *Solution:*
> Each message should be registered and during registration we can check that
> there is no registered message with the same type. Otherwise we should not
> start node and print error message.
> *Proposed implementation:*
> Instead of {{switch-case}} block new array of size 2^16 should be created.
> Each array cell should contain message default constructor reference or
> lambda which is responsible for message instance creation. So during message
> unmarshaling system just lookup ctor-ref or lambda from array by index and
> invoke it in order to create proper message instance.
> All message factory extensions and custom message should be registered at the
> same array before communication SPI will be started.
> If we try to register message with direct type for which already exists
> registered message then node start process must be terminated (directly, with
> out any failure handlers).
> If we get message with unknown direct type (there is now registered message
> for this direct type) then failure handler be invoked.
> It could affect perfromance so we should run microbenchmark for this change.
> Additional benefit of this change is improving code readability. At present
> we have the following code:
> {code:java}
> @Override public Message create(short type) {
> Message msg = null;
> switch (type) {
> case -61:
> msg = new IgniteDiagnosticMessage();
> break;
> case -53:
> msg = new SchemaOperationStatusMessage();
> break;
> // etc.
> }
> {code}
> After refactoring it will looks like:
> {code:java}
> private Supplier[] msgCtrs;
> public GridIoMessageFactory(MessageFactory[] ext) {
> this.ext = ext;
>registerMessage(-61, IgniteDiagnosticMessage::new)
>registerMessage(-53, SchemaOperationStatusMessage::new)
> }
> @Override public Message create(short type) {
> return msgCtros[type].get();
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
