You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apex.apache.org by ishark <gi...@git.apache.org> on 2015/09/25 02:36:18 UTC
[GitHub] incubator-apex-core pull request: SPOI-5173 #comment #resolve Adde...
GitHub user ishark opened a pull request:
https://github.com/apache/incubator-apex-core/pull/81
SPOI-5173 #comment #resolve Added changes for attribute serializable …
…check in dag.validate
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ishark/incubator-apex-core SPOI-5173
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-apex-core/pull/81.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #81
----
commit 0c524acaa499c4348ee93b410cd9ec0d266983cb
Author: ishark <is...@datatorrent.com>
Date: 2015-09-25T00:05:13Z
SPOI-5173 #comment #resolve Added changes for attribute serializable check in dag.validate
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-157 #comment #resolve Added...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/81#discussion_r40699218
--- Diff: engine/src/test/java/com/datatorrent/stram/plan/logical/LogicalPlanTest.java ---
@@ -683,7 +686,85 @@ public void testJdkSerializableOperator() throws Exception {
Assert.assertNotNull("port object null", o1Clone.inport1);
}
- private static class TestStreamCodec implements StreamCodec<Object> {
+ @Test
+ public void testAttributeValuesSerializableCheck() throws NoSuchFieldException, SecurityException, IllegalArgumentException, IllegalAccessException
+ {
+ LogicalPlan dag = new LogicalPlan();
+ Attribute<Object> attr = new Attribute<Object>(new TestAttributeValue(), new Object2String());
+ Field nameField = Attribute.class.getDeclaredField("name");
+ nameField.setAccessible(true);
+ nameField.set(attr, "Test_Attribute");
+ nameField.setAccessible(false);
+
+ assertNotNull(attr);
--- End diff --
Is this assert checking anything important. Couldn't it be removed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-157 #comment #resolve Added...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/81#discussion_r40698828
--- Diff: engine/src/test/resources/testTopology.json ---
@@ -25,7 +25,7 @@
"attributes": {
"UNIFIER_LIMIT": 8,
"STREAM_CODEC" : {
- "com.datatorrent.common.codec.JsonStreamCodec" : {}
+ "com.datatorrent.stram.plan.logical.LogicalPlanConfigurationTest$TestStreamCodec" : {}
}
--- End diff --
I Tested with the original JSON and the testLoadFromJson test failed because the attribute is not serializable. As it is now this PR is backwards incompatible. Could we make the JsonStreamCodec serializable to make the original JSON work or does that introduce other issues?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-157 #comment #resolve Added...
Posted by ilooner <gi...@git.apache.org>.
Github user ilooner commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/81#discussion_r40699028
--- Diff: engine/src/test/java/com/datatorrent/stram/plan/logical/LogicalPlanTest.java ---
@@ -683,7 +686,85 @@ public void testJdkSerializableOperator() throws Exception {
Assert.assertNotNull("port object null", o1Clone.inport1);
}
- private static class TestStreamCodec implements StreamCodec<Object> {
+ @Test
+ public void testAttributeValuesSerializableCheck() throws NoSuchFieldException, SecurityException, IllegalArgumentException, IllegalAccessException
+ {
+ LogicalPlan dag = new LogicalPlan();
+ Attribute<Object> attr = new Attribute<Object>(new TestAttributeValue(), new Object2String());
--- End diff --
new Attribute<Object> could be new Attribute<>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-157 #comment #resolve Added...
Posted by 243826 <gi...@git.apache.org>.
Github user 243826 commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/81#discussion_r40600289
--- Diff: engine/src/main/java/com/datatorrent/stram/plan/logical/LogicalPlan.java ---
@@ -1333,6 +1333,13 @@ public void validate() throws ConstraintViolationException
OperatorMeta.PortMapping portMapping = n.getPortMapping();
+ // Check all attributes are serializable
+ for (Entry<Attribute<?>, Object> entry : n.getAttributes().entrySet()) {
--- End diff --
just as a practice - when doing validation - please report all the validation errors all at once wherever possible instead of one at a time.
Here: can you collect all the attribute serialization problems and report them at once?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-157 #comment #resolve Added...
Posted by ishark <gi...@git.apache.org>.
Github user ishark closed the pull request at:
https://github.com/apache/incubator-apex-core/pull/81
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-157 #comment #resolve Added...
Posted by ishark <gi...@git.apache.org>.
Github user ishark commented on the pull request:
https://github.com/apache/incubator-apex-core/pull/81#issuecomment-145607263
Code changes have been merged with devel-3. Closing the pull request.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] incubator-apex-core pull request: APEX-157 #comment
#resolve Added...
Posted by Thomas Weise <th...@datatorrent.com>.
+1
On Tue, Sep 29, 2015 at 3:48 PM, Chetan Narsude <ch...@datatorrent.com>
wrote:
> This qualifies as a bug fix (or feature fix) which moves part of the
> validation from later phase to the earlier phase.
>
> Comments?
>
>
> On Tue, Sep 29, 2015 at 12:37 PM, ishark <gi...@git.apache.org> wrote:
>
> > Github user ishark commented on a diff in the pull request:
> >
> >
> >
> https://github.com/apache/incubator-apex-core/pull/81#discussion_r40718327
> >
> > --- Diff: engine/src/test/resources/testTopology.json ---
> > @@ -25,7 +25,7 @@
> > "attributes": {
> > "UNIFIER_LIMIT": 8,
> > "STREAM_CODEC" : {
> > - "com.datatorrent.common.codec.JsonStreamCodec" : {}
> > +
> >
> "com.datatorrent.stram.plan.logical.LogicalPlanConfigurationTest$TestStreamCodec"
> > : {}
> > }
> > --- End diff --
> >
> > Yes, the test fails with the original JSON. So I changed the stream
> > codec class in JSON.
> > It does make the change backword incompatible due to an additional
> > validation. However, if attribute values are not serializable, the app
> > currently fails during deployment. we need to catch attribute
> > serializability in validate phase itself.
> >
> >
> > ---
> > If your project is set up for it, you can reply to this email and have
> your
> > reply appear on GitHub as well. If your project does not have this
> feature
> > enabled and wishes so, or if the feature is enabled but not working,
> please
> > contact infrastructure at infrastructure@apache.org or file a JIRA
> ticket
> > with INFRA.
> > ---
> >
>
Re: [GitHub] incubator-apex-core pull request: APEX-157 #comment
#resolve Added...
Posted by Chetan Narsude <ch...@datatorrent.com>.
This qualifies as a bug fix (or feature fix) which moves part of the
validation from later phase to the earlier phase.
Comments?
On Tue, Sep 29, 2015 at 12:37 PM, ishark <gi...@git.apache.org> wrote:
> Github user ishark commented on a diff in the pull request:
>
>
> https://github.com/apache/incubator-apex-core/pull/81#discussion_r40718327
>
> --- Diff: engine/src/test/resources/testTopology.json ---
> @@ -25,7 +25,7 @@
> "attributes": {
> "UNIFIER_LIMIT": 8,
> "STREAM_CODEC" : {
> - "com.datatorrent.common.codec.JsonStreamCodec" : {}
> +
> "com.datatorrent.stram.plan.logical.LogicalPlanConfigurationTest$TestStreamCodec"
> : {}
> }
> --- End diff --
>
> Yes, the test fails with the original JSON. So I changed the stream
> codec class in JSON.
> It does make the change backword incompatible due to an additional
> validation. However, if attribute values are not serializable, the app
> currently fails during deployment. we need to catch attribute
> serializability in validate phase itself.
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>
[GitHub] incubator-apex-core pull request: APEX-157 #comment #resolve Added...
Posted by ishark <gi...@git.apache.org>.
Github user ishark commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/81#discussion_r40718327
--- Diff: engine/src/test/resources/testTopology.json ---
@@ -25,7 +25,7 @@
"attributes": {
"UNIFIER_LIMIT": 8,
"STREAM_CODEC" : {
- "com.datatorrent.common.codec.JsonStreamCodec" : {}
+ "com.datatorrent.stram.plan.logical.LogicalPlanConfigurationTest$TestStreamCodec" : {}
}
--- End diff --
Yes, the test fails with the original JSON. So I changed the stream codec class in JSON.
It does make the change backword incompatible due to an additional validation. However, if attribute values are not serializable, the app currently fails during deployment. we need to catch attribute serializability in validate phase itself.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-apex-core pull request: APEX-157 #comment #resolve Added...
Posted by 243826 <gi...@git.apache.org>.
Github user 243826 commented on a diff in the pull request:
https://github.com/apache/incubator-apex-core/pull/81#discussion_r40600446
--- Diff: engine/src/main/java/com/datatorrent/stram/plan/logical/LogicalPlan.java ---
@@ -1333,6 +1333,13 @@ public void validate() throws ConstraintViolationException
OperatorMeta.PortMapping portMapping = n.getPortMapping();
+ // Check all attributes are serializable
+ for (Entry<Attribute<?>, Object> entry : n.getAttributes().entrySet()) {
--- End diff --
Also what about port and dag attribute validations?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---