Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

APEXMALHAR-2517 relocate com.datatorrent packages to org.apache.apex #662

Merged
merged 2 commits into from Dec 28, 2017

Conversation

tweise
Copy link
Contributor

@tweise tweise commented Aug 14, 2017

No description provided.

@davidyan74
Copy link
Contributor

This is long overdue. LGTM.

@pramodin
Copy link
Contributor

Please give some time to look into this as there are a number of changes

@tweise
Copy link
Contributor Author

tweise commented Aug 17, 2017

@PramodSSImmaneni this PR is difficult to rebase but the changes are trivial (imports and checkstyle fixes). I would like to wrap it up in a day or two.

@amolhkekre
Copy link
Contributor

I am -1 on this as users this will break binary compatibility with current users, and will require code changes. For current users it is not backward incompatibility. The approach suggested in #664 is not adequate as it will require existing apps to stick to older version of Malhar. This change should be taken up in a major upgrade.

@tweise
Copy link
Contributor Author

tweise commented Aug 19, 2017

@amolhkekre I will call out the following WRT your veto on this PR:

  1. I did not see a contribution from you to the preceding 6 months old discussion thread
  2. You seem to suggest that suggested change (2 PRs) break binary compatibility ("as users this will break binary compatibility with current users"). However, the proposed solution doesn't. Granted current guarantees in malhar are confusing because in reality it is evolving and stricter guarantees would mostly bring development to a halt. However, this has been discussed on the list and when it comes to a veto on the PR then IMO the onus is on you to have the context.

@vrozov
Copy link
Member

vrozov commented Nov 20, 2017

Please resolve conflicts and update pom.

@amolhkekre
Copy link
Contributor

amolhkekre commented Nov 20, 2017 via email

pom.xml Outdated
@@ -139,7 +139,10 @@
<excludes>
<exclude>@org.apache.hadoop.classification.InterfaceStability$Evolving</exclude>
<exclude>@org.apache.hadoop.classification.InterfaceStability$Unstable</exclude>
<exclude>com.datatorrent.contrib.parquet</exclude>
<exclude>com.datatorrent.lib</exclude>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those excludes required if the major version is changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they won't be needed. Should I make the version change in the same commit?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My take - yes. Is there a reason not to?

@tweise
Copy link
Contributor Author

tweise commented Dec 26, 2017

@vrozov added version number change and also removed obsolete samples directory. Will squash these commits once approved.

@@ -19,14 +19,14 @@

package com.datatorrent.apps.copy;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be relocated to org.apache.malhar.apps.copy (possibly as part of a follow-up PR). If yes, can you file JIRA?

Copy link
Member

@vrozov vrozov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM +1. Please squash commits.

@tweise
Copy link
Contributor Author

tweise commented Dec 28, 2017

test this please

1 similar comment
@tweise
Copy link
Contributor Author

tweise commented Dec 28, 2017

test this please

@vrozov vrozov merged commit 0d98d05 into apache:master Dec 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants