> On Oct. 13, 2014, 2:20 p.m., Jarek Cecho wrote: > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java, > > lines 20-55 > > <https://reviews.apache.org/r/26657/diff/1/?file=719686#file719686line20> > > > > Is this change intentional? > > Veena Basavaraj wrote: > yes it is, it is always hard to do merges with every single import added. > > I would like to understand if this is performance concern/ nitpick or is > there some standards we follow for the import order. I did not see it > documented anywhere, would be nice to have a pointer to the code style > expected. > > Jarek Cecho wrote: > I usually don't like asterisks import as they are masking what is > actually used in the module. In this particular case where it's obviously > that we're trying to get all the various queries, I don't mind as it's just > following the very close relationship between those two classes. I was asking > to understand whether it's intentional change or some automatic IDE change as > there are IDEs that are automatically mergin multiple import statements > together.
as you have very nicely pointed out, every single constant in that file is used. So might as well say * :) > On Oct. 13, 2014, 2:20 p.m., Jarek Cecho wrote: > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java, > > lines 205-206 > > <https://reviews.apache.org/r/26657/diff/1/?file=719686#file719686line205> > > > > The reason why we have the code duplicated here, is that if somebody > > will unintentionally change the code in DerbyRepositoryHandler, then all > > tests will start breaking and he will realize that the change has been > > invalid as it requires an upgrade path. Hence unless we will somehow > > accomplish this goal differently, I would prefer to keep this particular > > code duplication. > > Veena Basavaraj wrote: > There are other ways to handle such things, than duplicating code, I am > happy to create a ticket for this and fix it the right way, instead of > debating. I really dont like how copy paste is considered a solution for > things. > > Veena Basavaraj wrote: > BTW, when I made changed for SQOOP-1498, I did not do the upgrade logic > the right way, neither the tests broke nor did I find any documentation on > the right way to upgrade. So I am not sure if you still think the copy paste > magic i helping guard a developer to not break functionality. > > Jarek Cecho wrote: > I'm open to use better approach if we have any. I'm not sayin that this > is 100% solution, but it's also non 0%. Feel free to create follow up JIRA. I will update the comment with the JIRA, so we remove the duplication of the code between the actual code and tests and solve the actual issue of breaking tests when upgrade is not done. That clearly does not happen today. Try changing the name of field on any table and no tests will break. > On Oct. 13, 2014, 2:20 p.m., Jarek Cecho wrote: > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java, > > lines 96-97 > > <https://reviews.apache.org/r/26657/diff/1/?file=719686#file719686line96> > > > > We should keep this constant here as well - again it's intentionally > > duplicated here, so that the developer is intentionally changing it on two > > places to realize need to do the upgrade. > > Veena Basavaraj wrote: > why would we have duplication like this in the code? Test code usually > uses the same constants. This is asking for trouble to have duplicate code in > test, which I have already seen is true in many places in the sqoop code, > refactoring breaks it > > Jarek Cecho wrote: > We are sharing pretty much almost all the constants already. We have few > exceptions where we believe that devs should double check before changing > them to ensure that we won't break important functionality such as upgrades. > One such example is here - we want to allow dev to just change the number and > get away with it, we want the test to break if the change is not intentional. > And if it's intentional, then the dev have to provide an upgrade path which > will require much more changes and will render this one small constant > insignificant. I am not sure how having duplicate code helps. As I said below if the goal is for tests to break, lets make sure that happens. I am yet to see a test breaking if I rename a field. I infact did it in the SQOOP-1498 and no tests broke. So lets be clear what we are guarding for. > On Oct. 13, 2014, 2:20 p.m., Jarek Cecho wrote: > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInputTypes.java, > > lines 20-56 > > <https://reviews.apache.org/r/26657/diff/1/?file=719689#file719689line20> > > > > Nit: Seems as unnecessary changes? > > Veena Basavaraj wrote: > same as above, is there a import order imposed so I can use that as a > standard in the IDE. If we have more developers contributing to the code > base, projects use a style guide as a standard to avoid debating on such > things:). Time well spent on real issues. > > Jarek Cecho wrote: > I don't think that we have any style guide with regards to the imports > right now. I'm open to include it, but I would be sceptical to apply it > everywhere immediatelly. My concern here will pretty much follow what I've > mentioned elsewhere - this code will make the following commands either > unuseable or troublesome: > > 1) git blame > 2) git cherrypick > > Where both commands are very important in long term project where > multiple developers are working on the code base and trying to figure out > what/when/why. Dont think we need to apply it everywhere, as new files change we will apply. why is git blame hard if we change and stick to a import order. I am usign the default order of the IDE right now and this cannot be same for all IDEs. - Veena ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26657/#review56443 ----------------------------------------------------------- On Oct. 13, 2014, 1:03 p.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26657/ > ----------------------------------------------------------- > > (Updated Oct. 13, 2014, 1:03 p.m.) > > > Review request for Sqoop. > > > Repository: sqoop-sqoop2 > > > Description > ------- > > fix the upgrade logic for 1498 changes. > While fixing the upgrade logic, added comments on how the upgrade logic works. > -renamed test methods to reflect what they are upgrading and what they are > infact testing. > -renamed the internals to exactly mean "schema" upgrades, since we only > allow/ do scheme upgrades the repository > -Also updated the names on the repository api to clearly state the difference > between upgrade and update operations on connector / driver. > > > Diffs > ----- > > core/src/main/java/org/apache/sqoop/repository/JdbcRepository.java 3ade247 > core/src/main/java/org/apache/sqoop/repository/JdbcRepositoryHandler.java > 97de893 > core/src/main/java/org/apache/sqoop/repository/Repository.java 95c7a4d > core/src/main/java/org/apache/sqoop/repository/RepositoryManager.java > c2f8505 > core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java > e6e4760 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoConstants.java > 74e41df > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepoError.java > 0f0f7c4 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbyRepositoryHandler.java > 10a7b1a > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaConstants.java > cf6e657 > > repository/repository-derby/src/main/java/org/apache/sqoop/repository/derby/DerbySchemaQuery.java > 56ea147 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/DerbyTestCase.java > 9316687 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestConnectorHandling.java > fc95222 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestDriverHandling.java > d597bd8 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInputTypes.java > 260c2a9 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestInternals.java > 0eb9df4 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestJobHandling.java > 01a05b2 > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestLinkHandling.java > bbfe5bb > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestRespositorySchemaUpgrade.java > PRE-CREATION > > repository/repository-derby/src/test/java/org/apache/sqoop/repository/derby/TestSubmissionHandling.java > 8402d8c > > Diff: https://reviews.apache.org/r/26657/diff/ > > > Testing > ------- > > yes > > > Thanks, > > Veena Basavaraj > >
