Skip to content

WIP - convert translators to lambdas#1676

Closed
codebyneesh wants to merge 2 commits intoapache:mainfrom
codebyneesh:accumulo1533
Closed

WIP - convert translators to lambdas#1676
codebyneesh wants to merge 2 commits intoapache:mainfrom
codebyneesh:accumulo1533

Conversation

@codebyneesh
Copy link

@codebyneesh codebyneesh commented Aug 11, 2020

Currently WIP to monitor progress on issue #1533 .

@codebyneesh codebyneesh marked this pull request as draft August 11, 2020 15:50
HashSet<KeyExtent> fullScans =
new HashSet<>(Translator.translate(scanResult.fullScans, Translators.TKET));
unscanned.keySet().removeAll(fullScans);
// translate full scans and remove them from unscanned
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this comment one line down with the indent removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the indent here.

Copy link
Contributor

Choose a reason for hiding this comment

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

running mvn compile locally will format all of the source code I think and that may fix this up

Copy link
Member

Choose a reason for hiding this comment

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

mvn compile may have issues with dependency resolution on the non-java modules (such as the native map module, which doesn't produce a java jar, but rather a tar.gz artifact). A better command, that won't have these issues in dependency resolution between sibling modules is mvn package -DskipTests. It does basically the same thing, but ensures that all artifacts are built in sibling modules, so there's no issues resolving non-java dependencies from sibling modules in the multi-module reactor build.


}


Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these empty lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

We've already discussed this in person, but I'm including a comment here so anyone else can chime in; these kinds of transformation operations are good candidates for being moved to a separate utility class that supports multiple types of collection transformations using lambdas and function parameters. This would allow shorter function calls to improve readability, as well as make it available to the project. I'd be happy to help out with this effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

running mvn compile locally will format all of the source code I think and that may fix this up


Map<TKeyExtent,List<TRange>> thriftTabletRanges = Translator.translate(requested,
Translators.KET, new Translator.ListTranslator<>(Translators.RT));
Map<TKeyExtent, List<TRange>> thriftTabletRanges = requested.entrySet().stream()
Copy link
Contributor

Choose a reason for hiding this comment

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

With these changes, is it possible to remove any of the constants like Translators.KET?

Copy link
Contributor

@jzgithub1 jzgithub1 left a comment

Choose a reason for hiding this comment

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

Well done @plainolneesh. This starting to shape up well .

@EdColeman
Copy link
Contributor

If you run mvn clean verify javadoc:javadoc -DskipTests that will execute most of the qa checks locally. For example the file https://github.com/apache/accumulo/pull/1676/files#diff-e09bb0e400bc6c61f3735dce25aebc78 is missing the apache licence and the RAT checker will catch that.

@ctubbsii
Copy link
Member

Closing, as this was superseded by #1802

@ctubbsii ctubbsii closed this Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Comments