New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Eclipse Collections as a dependency. #134

Merged
merged 5 commits into from Mar 2, 2018

Conversation

Projects
None yet
3 participants
@itohiro73
Contributor

itohiro73 commented Feb 15, 2018

There are couple of open issues I want to consult with @gs-rezaem . Please kindly advise.

  • In #30 , Moh suggested to try getting rid of GSC impl dependency, but there are too many dependencies for FastList/UnifiedSet/UnifiedMap as internal storage, so I left them as is for now.
  • In #30, Moh suggested below for ByteArraySet, but this caused conflict in get(Object) and get(K), also signature conflict in collect(). So I didn't touch this class for now. Please advise how you want to handle this class.

For ByteArraySet, extend the EC UnifiedSetWithHS and implement GSC MutableMap.

  • MithraFastList and AdhocFastList extend FastList. Please advise how you want to handle these classes.
  • AbstractDatedCache.MatchAllAsOfDatesProcedureForMany#getResult() returns FastList. Please advise how you want to handle this.
  • MasterSyncResult#getBuffers() returns FastList. Please advise how you want to handle this.
  • MithraCompositeList#getLists() returns FastList. Please advise how you want to handle this.
  • AsOfEqualityChecker#getMsiPool() returns UnifiedMap. Please advise how you want to handle this.
  • ConcurrentIntObjectHashMap#parallelForEachValue() takes List<IntObjectProcedure<V>> in its signature. Introducing EC equivalent method would cause same erasure issue. Please advise how you want to handle this.
  • ListFactory has couple of create() methods that return FixedSizeList. We can either have GSC version or EC version for this class. Making it EC version may cause wide impact as it's used widely in reladomo. Please advise how you want to handle this class.
@gs-rezaem

This comment has been minimized.

Contributor

gs-rezaem commented Feb 15, 2018

I think replacing the internal usages is pretty important because it'll flush out some of the interactions with the pseudo-public methods (see below) that you're having trouble with.

For ByteArraySet, use the same solution as primitives: create a new one and deprecate the old. Use the com.gs.reladomo.util package for the new one.

The other methods you've listed is what I would call "pseudo-public". Convert all of them to EC.

@itohiro73

This comment has been minimized.

Contributor

itohiro73 commented Feb 17, 2018

Thanks @gs-rezaem for your comment! It is very helpful.

I've added a commit to replace GSC impl with EC impl. Now only 3 impl usages are left after the change. Because of these, we still cannot get rid of GSC lib dependency itself.

  • ListAbstract.jsp for asGscList()
  • AggregateList#getAttributeAsGscIntSet(String) that constructs IntHashSet in it
  • Old ByteArraySet that implements GSC UnifiedSetWithHashingStrategy.

FYI, along with converting GSC impl to EC, I've optimized imports as well, so it may look more changes than necessary in imports.

Please let me know for further comments or suggestions.

@itohiro73

This comment has been minimized.

Contributor

itohiro73 commented Feb 19, 2018

Fixed issues in hiroshi_ito.dco commit.

  • Fix file name typo: hiroshi_ito.cdo => hiroshi_ito.dco
  • Fix missing replacement: <your name>.dco => hiroshi_ito.dco
@gs-rezaem

This comment has been minimized.

Contributor

gs-rezaem commented Feb 19, 2018

asGscList is in the generated code and it's optional, so it's not a real dependency.
The other two are so rare in production, that I wonder if people would mind fixing a couple of places in their code when upgrading.
Can you put together a guide to changing a project over from GSC to EC? I can think of two steps:

  • replace imports
  • replace asGscList with asEcList
@itohiro73

This comment has been minimized.

Contributor

itohiro73 commented Feb 20, 2018

Sure, let me put together a guide.

As for two remaining GSC dependencies, do you want me to get rid of them or leave them as is for now?

@gs-rezaem

This comment has been minimized.

Contributor

gs-rezaem commented Feb 20, 2018

Let's remove the two.

@itohiro73

This comment has been minimized.

Contributor

itohiro73 commented Mar 1, 2018

Now all GSC impl dependencies have been removed 👍.

I added a guide (GSC_TO_EC_MIGRATION_GUIDE.md) to migrate from GSC to EC, too. I'd assume there is going to be a link to this guide from CHANGELOG.md as part of 1.7.0 release.

Please let me know for any feedback.

```
2. Replace `asGscList()` with `asEcList()` in your code.
3. Replace `com.gs.collections.*` imports with `eclipse.collections.*` in your code

This comment has been minimized.

@donraab

donraab Mar 1, 2018

Collaborator

I think this should be
Replace com.gs.collections.* imports with org.eclipse.collections.* in your code

This comment has been minimized.

@itohiro73

itohiro73 Mar 1, 2018

Contributor

@donraab Good catch! I'll fix it.

This comment has been minimized.

@itohiro73

itohiro73 Mar 1, 2018

Contributor

Fixed!

@gs-rezaem gs-rezaem merged commit 516263c into goldmansachs:master Mar 2, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gs-rezaem

This comment has been minimized.

Contributor

gs-rezaem commented Mar 2, 2018

Merged with some minor changes. Thanks for this huge piece of work 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment