Skip to content
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

CAMEL-14910: Bundling of the heavily distributed components #3879

Closed
wants to merge 27 commits into from

Conversation

AemieJ
Copy link
Contributor

@AemieJ AemieJ commented Jun 1, 2020

  • Changes made to gulpfile.js but when running the mvn clean install, it results in an error each time and my git commits are perhaps overwritten.
  • @djencks, I think it's best to rebase my branch on top of yours as I have tried with CAMEL-14910-copy branch within your repo and it resulted in the same FAILURE message.
  • If the java build is working perfectly for you, and the gulpfile works for mine, it's a good choice to go with rebasing.

@djencks
Copy link
Contributor

djencks commented Jun 1, 2020

Could you run mvn clean install -DskipTests -e and paste the error and stack trace and any other information that seems relevant?

@AemieJ

This comment has been minimized.

@djencks
Copy link
Contributor

djencks commented Jun 1, 2020

That’s not a problem with our work. Googling, it seems to be a problem with mvel on jdks >11. I’m u sing Jdk 11, what are you using?

@AemieJ
Copy link
Contributor Author

AemieJ commented Jun 1, 2020

That’s not a problem with our work. Googling, it seems to be a problem with mvel on jdks >11. I’m u sing Jdk 11, what are you using?

jdk 14. I will use jdk 11 and check.

@AemieJ
Copy link
Contributor Author

AemieJ commented Jun 1, 2020

@djencks I am facing the same build problem at CAMEL:: File even for jdk 11

@djencks
Copy link
Contributor

djencks commented Jun 1, 2020

I'm often confused by what jdk maven is using. If you run mvn --version does it say you have succeeding in getting it to run jdk 11 :-) ?

@AemieJ
Copy link
Contributor Author

AemieJ commented Jun 2, 2020

@djencks That solved the problem, however, the build works for components however it fails at CAMEL:: Kudu

 Could not resolve dependencies for project org.apache.camel:camel-kudu:jar:3.4.0-SNAPSHOT: Failure to find org.apache.kudu:kudu-binary:jar:windows-x86_64:1.10.0 in https://repo1.maven.org/maven2/ was cached in the local repository, resolution will not be reattempted until the update interval of central has elapsed or updates are forced

The pom.xml is the same as the parent repo for camel-kudos.

@djencks
Copy link
Contributor

djencks commented Jun 2, 2020

I checked on maven central (https://search.maven.org/artifact/org.apache.kudu/kudu-binary/1.10.0/jar) and it looks like there are only binaries for linux and osx.
Unless you can find another solution, what I suggest is:

  • Claus just applied my PR, so if you rebase on master your PR will be working with it and able to be applied directly.
  • Tomorrow (perhaps 10 hours) I'll work with your PR, run the build to generate the final adoc files, and either add a commit to your PR (I'm not sure if that's possible) or make another PR with the changes. Hopefully we can then get them committed!

@AemieJ
Copy link
Contributor Author

AemieJ commented Jun 2, 2020

@djencks I was using windows till now to run maven, I will use Linux and try to make it work out.

@AemieJ
Copy link
Contributor Author

AemieJ commented Jun 2, 2020

@djencks I have done the java build and generated the nav.adoc the file however I found a slight issue in it though I manually resolved it. The issue was that aws-summary.adoc and aws2-summary.adoc when generated within the docs, for the nav.adoc they came in continuous and not segmented to their respective bundle. It appeared as followed within the nav.adoc:

- aws-summary.adoc
- aws2-summary.adoc 

@djencks
Copy link
Contributor

djencks commented Jun 2, 2020

We need a better sorting algorithm.
If the summary-group was always the same as the docTitle, then I think the comparison for the sort could be (lets assume everything is uppercased already):
comparing (docTitle1, group1) and (docTitle2, group2):

  • notice if they are equal, return 0
  • if group1 and group2 are null, then return comparison docTitle1 and docTitle2
  • if group1 and group2 are both present, compare them
    -- if not equal, return comparison
    -- if equal, return comparison of docTitle1 and docTitle2
  • if only group1 is present:
    -- if group1 === docTitle2, then the first item is after the second item.
    -- otherwise, return the comparison of group1 and docTitle2.
  • if only group2 is present:
    -- if docTitle1 === group2, then the first item is before the second item.
    -- otherwise, return the comparison of docTitle1 and group2.

Does this make sense? Assuming it does, do you want to write the code for this or shall I?

@AemieJ
Copy link
Contributor Author

AemieJ commented Jun 2, 2020

@djencks By group you mean the sorting of summary-group right? 'cause that's the one where the issue is arising. Also for the 4th and 5th case, not all have summary-group attribute so when would the case be that group1 will be equal to docTitle2 and same equality thing for the 5th case.

Also, in addition, I wanted to ask in the nav.adoc generation why is aws2 stated before aws?

@djencks
Copy link
Contributor

djencks commented Jun 2, 2020

I could have been a lot clearer :-)
I'm suggesting removing summary-group and using docTitle instead.
Then "summary-group" (docTitle) will always be present.
If we allow summary-group to differ from docTitle then we will need a much more complicated sorting algorithm, which would probably start by basically replacing summary-group values and the corresponding group values with docTitle.

The items in the current nav are actually sorted in lexicographical order, but that doesn't correspond to our desired grouping.

** xref:aws-summary.adoc[AWS]
** xref:aws2-summary.adoc[AWS 2]
*** xref:aws2-cw-component.adoc[AWS 2 CloudWatch]
*** xref:aws2-ddb-component.adoc[AWS 2 DynamoDB]
...
*** xref:aws2-sqs-component.adoc[AWS 2 Simple Queue Service (SQS)]
*** xref:aws2-translate-component.adoc[AWS 2 Translate]
*** xref:aws-cw-component.adoc[AWS CloudWatch]
*** xref:aws-ddb-component.adoc[AWS DynamoDB]

2 comes before C

@AemieJ
Copy link
Contributor Author

AemieJ commented Jun 2, 2020

Alright, summary group is the same as docTitle only. It makes sense now.

@AemieJ
Copy link
Contributor Author

AemieJ commented Jun 3, 2020

@djencks I wrote a sorting function that works and also weren't we planning to use summary-group to create an index-list to auto-generate the list within the -summary.adoc instead of manual typing.

@djencks
Copy link
Contributor

djencks commented Jun 3, 2020

  • I left out an important sorting case, when group1 <> group2 but both are present. This results in the sub-list being unsorted.
  • Please change the group for the AWS 2 sublist rather than the docTitle for the parent.
  • Use === and !== rather than == and !=
  • Use const rather than let or var for constants
  • line 166 is unnecessary and wrong (if present, it would need the symmetrical other case with the titles in the other order).
  • The ternary operator looks best when there are spaces surrounding both the ? and : (at least that's what I'm used to).
  • I think the sorting code, which is rather complicated and hard to understand, will be easier to read if we compute all the things we'll be comparing at the top rather than over and over again, something like:
    if (file1 === file2) return 0
    var t
    const group1 = (t = groupFrom(file1)) && t.toUpperCase()
    const group2 = (t = groupFrom(file2)) && t.toUpperCase()
...//same for title1, title2

Locally, I'm seeing an additional generated change to AWS x-ray. Since this is an "other" component I don't think it needs a group. I'm not sure why you wouldn't see this.

@AemieJ
Copy link
Contributor Author

AemieJ commented Jun 3, 2020

@djencks I thought I didn't include AWS Xray, other than that the code can look more sophisticated and optimized & for the first could you elobarate, we included the case where group1 and group2 are both present and if they are unequal, a comparison to take place.

@djencks
Copy link
Contributor

djencks commented Jun 3, 2020

When I look at the aws xray component adoc, it appears to have a ":group: AWS" line that I'm pretty sure you added. I don't think it's needed.

Have you noticed that the sublists are not sorted correctly? I was explaining what is missing. There are two cases when both group1 and group2 are present, either they are equal or not. We need to deal appropriately with both in the sort.

@AemieJ
Copy link
Contributor Author

AemieJ commented Jun 4, 2020

@djencks Yea, I will correct that. I missed out on a condition for both groups not null. I will modify it.
Also, throughout the file == & != is used rather than === & !==, I personally prefer the later but I chose former to maintain consistency with the whole code of gulpfile.

@djencks
Copy link
Contributor

djencks commented Jun 4, 2020

This looks correct to me, although I didn't try it. Apparently there are some merge conflicts, but I'm in favor of applying this if you can fix those.

Perhaps you would like to fix the == > === etc for the whole file in a separate issue? I suspect everyone has been used to looking at java and didn't notice the problem here -- I certainly didn't!

@AemieJ
Copy link
Contributor Author

AemieJ commented Jun 4, 2020

@djencks I don't know why the checks are failing, though it doesn't show any conflicts with the branch.
And yes, that can be resolved in a different PR, the code structure for the js file.

@AemieJ
Copy link
Contributor Author

AemieJ commented Jun 5, 2020

@davsclaus, @djencks I performed mvn clean install command and it builds successfully. I am not sure why the check is failing here.

@djencks
Copy link
Contributor

djencks commented Jun 6, 2020

The maven source check problem is unrelated to this work. When this work is merged I think it should have 3 commits:

  • the gulpfile changes
  • the manual changes to components .adocs adding the "group" attribute
  • the generated changes from the maven build and the gulpfile build.

When I tried to investigate the "can't rebase" warning I see from GitHub, and tried git rebase master on your branch, there was certainly a merge conflict, but I couldn't understand it. I think if you start by running git rebase -i c9d5377fd7a7888f9edeee649ead99084cccf64e and reordering and squashing your commits into the above 3 it will be easier to see if there are any actual problems.

@AemieJ
Copy link
Contributor Author

AemieJ commented Jun 6, 2020

@djencks I did the process with the following 3 commits in the PR#3893 respect with the master branch commits.

@oscerd oscerd closed this Jun 8, 2020
@oscerd
Copy link
Contributor

oscerd commented Jun 8, 2020

Closing this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants