GEODE-3987: enforce GatewayReceiver uniqueness per member. by jujoramos · Pull Request #1086 · apache/geode

jujoramos · 2017-11-22T10:52:00Z

Enforcements added in the different creation entry points to prevent
the existence of more than one gateway receiver per member.

Added several XML parsing tests.
Added integration and Junit tests.
DTD allows 0 or 1 occurrences of the element.
XSD allows 0 or 1 occurrences of the element.
GatewayReceiverFactory checks before creating new gateway receiver.
Existing tests modified to use new MemberVM/GfshCommandRule features.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically develop)?
Is your initial contribution a single, squashed commit?
Does gradlew build run cleanly?
Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

Enforcements added in the different creation entry points to prevent the existence of more than one gateway receiver per member. - Added several XML parsing tests. - Added integration and Junit tests. - DTD allows 0 or 1 occurrences of the <gateway-receiver> element. - XSD allows 0 or 1 occurrences of the <gateway-receiver> element. - GatewayReceiverFactory checks before creating new gateway receiver. - Existing tests modified to use new MemberVM/GfshCommandRule features.

nabarunnag · 2017-11-22T19:20:44Z

.../test/java/org/apache/geode/internal/cache/wan/GatewayReceiverXmlParsingValidationsTest.java

+@PrepareForTest(WANServiceProvider.class)
+@PowerMockRunnerDelegate(Parameterized.class)
+@PowerMockIgnore({"javax.management.*", "javax.security.*", "*.IntegrationTest"})
+public class GatewayReceiverXmlParsingValidationsTest {


Could this be renamed to GatewayReceiverXmlParsingValidationsJUnitTest.java

nabarunnag · 2017-11-22T19:57:25Z

...ernal/cache/wan/GatewayReceiverXmlParsingValidationsTest.correctConfiguration[DTD].cache.xml

+  limitations under the License.
+-->
+
+<!DOCTYPE cache PUBLIC


Sorry small nit pick stuff.. can these Gemstone headers be removed now

Hey @nabarunnag,
What do you mean by "remove these Gemstone headers" here?.
The file cache8_0.dtd still references the old GemStone Systems as well, so these files should use correct declaration, am I missing something?:

This is the XML DTD for the GemFire distributed cache declarative caching XML file. All declarative cache files must include a DOCTYPE of the following form: <!DOCTYPE cache PUBLIC "-//GemStone Systems, Inc.//GemFire Declarative Caching 8.0//EN" "http://www.gemstone.com/dtd/cache8_0.dtd"> If you are declaring a client cache then use this DOCTYPE: <!DOCTYPE client-cache PUBLIC "-//GemStone Systems, Inc.//GemFire Declarative Caching 8.0//EN" "http://www.gemstone.com/dtd/cache8_0.dtd">

@jujoramos You're right, i was not very familiar with the compatibility issues with older systems.

jhuynh1 · 2017-11-22T20:27:42Z

geode-core/src/main/java/org/apache/geode/internal/i18n/LocalizedStrings.java

  public static final StringId AbstractGatewaySender_ENQUEUEING_SYNCHRONIZATION_EVENT =
      new StringId(6665, "{0}: Enqueueing synchronization event: {1}");
+  public static final StringId GatewayReceiver_ALREADY_EXISTS =
+      new StringId(6665, "A Gateway Receiver already exists on this member.");


This StringId probably needs to be bumped to 6666 or something that isn't already being used.

It was actually discussed on the dev list that we probably can add this string to the message itself and not have to update LocalizedStrings any longer.

Thanks @jhuynh1,
I'll directly remove the constant from the LocalizedStrings class, missed that discussion in the dev list.

Changes requested by reviewers have been incorporated.

boglesby · 2017-11-27T21:05:00Z

geode-core/src/main/resources/org/apache/geode/cache/doc-files/cache8_0.dtd

    gateway-hub*,
    gateway-sender*,
-    gateway-receiver*,
+    gateway-receiver?,


I don't think any customer is configuring multiple gateway receivers per node because it would throw an mbean exception, but are we allowed to just change the dtd like this? Do we need to 'deprecate' multiple receivers per node?

Hello @boglesby,

As you said, I don't think anybody is actually trying to configure more than one gateway-receiver per node, the member will just throw an exception and shutdown when trying to register the MBean. That's why I added the validation int he DTD/XSD as well, primarily to fail fast and allow earlier detection of the issue for people using XML modeling tools and/or editors.
Just for the record, I was also concerned about "breaking the backward compatibility" by modifying these files, so I had a discussion with @jhuynh1 / @nabarunnag and they agreed with the change. Bottom line, even if we don't change the DTD/XSD the member will fail when configuring more than one gateway-receiver so we're not actually breaking the backward compatibility, but just making sure the product is configured as it should.
Just my two cents, let me know what you think.
Cheers.

boglesby · 2017-11-27T21:06:29Z

geode-core/src/main/resources/META-INF/schemas/geode.apache.org/schema/cache/cache-1.0.xsd

        </xsd:element>

-        <xsd:element maxOccurs="unbounded" minOccurs="0" name="gateway-receiver">
+        <xsd:element maxOccurs="1" minOccurs="0" name="gateway-receiver">


I don't think any customer is configuring multiple gateway receivers per node because it would throw an mbean exception, but are we allowed to just change the xsd like this? Do we need to 'deprecate' multiple receivers per node?

Hello @boglesby,

As you said, I don't think anybody is actually trying to configure more than one gateway-receiver per node, the member will just throw an exception and shutdown when trying to register the MBean. That's why I added the validation int he DTD/XSD as well, primarily to fail fast and allow earlier detection of the issue for people using XML modeling tools and/or editors.
Just for the record, I was also concerned about "breaking the backward compatibility" by modifying these files, so I had a discussion with @jhuynh1 / @nabarunnag and they agreed with the change. Bottom line, even if we don't change the DTD/XSD the member will fail when configuring more than one gateway-receiver so we're not actually breaking the backward compatibility, but just making sure the product is configured as it should.
Just my two cents, let me know what you think.
Cheers.

nabarunnag reviewed Nov 22, 2017

View reviewed changes

jhuynh1 requested changes Nov 22, 2017

View reviewed changes

GEODE-3987: enforce GatewayReceiver uniqueness per member.

8d438f1

Changes requested by reviewers have been incorporated.

boglesby reviewed Nov 27, 2017

View reviewed changes

boglesby merged commit ba8f963 into apache:develop Nov 29, 2017

jujoramos deleted the feature/GEODE-3987 branch May 24, 2019 23:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GEODE-3987: enforce GatewayReceiver uniqueness per member.#1086

GEODE-3987: enforce GatewayReceiver uniqueness per member.#1086
boglesby merged 2 commits intoapache:developfrom
jujoramos:feature/GEODE-3987

jujoramos commented Nov 22, 2017

Uh oh!

nabarunnag Nov 22, 2017

Uh oh!

jujoramos Nov 22, 2017

Uh oh!

nabarunnag Nov 22, 2017

Uh oh!

jujoramos Nov 22, 2017

Uh oh!

nabarunnag Nov 28, 2017 •

edited

Loading

Uh oh!

jhuynh1 Nov 22, 2017 •

edited

Loading

Uh oh!

jujoramos Nov 22, 2017

Uh oh!

boglesby Nov 27, 2017

Uh oh!

jujoramos Nov 27, 2017

Uh oh!

boglesby Nov 27, 2017

Uh oh!

jujoramos Nov 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

jujoramos commented Nov 22, 2017

For all changes:

Note:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nabarunnag Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhuynh1 Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nabarunnag Nov 28, 2017 •

edited

Loading

jhuynh1 Nov 22, 2017 •

edited

Loading