Skip to content

Conversation

@ivanzlenko
Copy link
Contributor

What changes were proposed in this pull request?

Jgrapht library was updated to the most recent version.

What is the link to the Apache JIRA

HDDS-10503

How was this patch tested?

New unit tests added to cover PrintableGraph class.

@@ -0,0 +1,75 @@
package org.apache.ozone.graph;
Copy link
Contributor

@adoroszlai adoroszlai Mar 11, 2024

Choose a reason for hiding this comment

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

License header is needed in all files. Example:

/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
* <p>
* http://www.apache.org/licenses/LICENSE-2.0
* <p>
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we need more informative output in rat.sh and mark build failed :)
~/projects/opensource/ozone hadoop-hdds/rocksdb-checkpoint-differ/target/rat.txt: !????? /home/izlenko/projects/opensource/ozone/hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/graph/TestPrintableGraph.java izlenko:~/projects/open
I've totally missed that.

Copy link
Contributor

@adoroszlai adoroszlai Mar 11, 2024

Choose a reason for hiding this comment

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

I guess we need more informative output in rat.sh

Agreed.

and mark build failed

I don't see any problem there, the check failed: https://github.com/ivanzlenko/ozone/actions/runs/8233075581/job/22511818434

Copy link
Contributor Author

@ivanzlenko ivanzlenko Mar 11, 2024

Choose a reason for hiding this comment

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

[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  17.799 s
[INFO] Finished at: 2024-03-11T17:44:03+05:00
[INFO] ------------------------------------------------------------------------
[INFO] 26 goals, 26 executed
~/projects/opensource/ozone
hadoop-hdds/rocksdb-checkpoint-differ/target/rat.txt: !????? /home/izlenko/projects/opensource/ozone/hadoop-hdds/rocksdb-checkpoint-differ/src/test/java/org/apache/ozone/graph/TestPrintableGraph.java

Whole output from console.

Copy link
Contributor

Choose a reason for hiding this comment

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

rat.sh exits with error code.

$ echo $?
1

BUILD SUCCESS is printed only because hadoop-hdds and hadoop-ozone are checked separately, the latter is successful. This will be improved in #6358, which changes the build to run a single Maven command for the whole project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@myskov myskov left a comment

Choose a reason for hiding this comment

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

Thanks @ivanzlenko for the patch!

}

@ParameterizedTest
@MethodSource("graphTypes")
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using EnumSource, it will allow:

  • get rid of graphTypes() method
  • No need in modifying this test if a new value is added to the enum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for feedback! Changed to EnumSource

@ivanzlenko ivanzlenko requested a review from myskov March 11, 2024 13:45
@adoroszlai
Copy link
Contributor

Unfortunately JGraphT 1.5.x requires Java 11.

https://sourceforge.net/p/jgrapht/news/2020/06/jgrapht-version-150-released/

@ivanzlenko
Copy link
Contributor Author

@adoroszlai yeah, I've downgraded it to 1.4.0. It still contains newer version of jgraphx, so it should be okay.

@adoroszlai
Copy link
Contributor

Thanks @ivanzlenko for updating the patch.

It looks like new version has slightly different transitive dependencies. Please update LICENSE.txt and jar-report.txt:
https://github.com/apache/ozone/actions/runs/8234045319/job/22516088723?pr=6364#step:5:10

@adoroszlai adoroszlai changed the title HDDS-10503. Upgrade jgrapht dependency. HDDS-10503. Bump jgrapht to 1.4.0 Mar 11, 2024
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @ivanzlenko for the patch, LGTM.

Copy link
Contributor

@myskov myskov left a comment

Choose a reason for hiding this comment

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

Thanks for the update @ivanzlenko.
Thanks for reviewing @adoroszlai.

@myskov myskov merged commit d68ea97 into apache:master Mar 11, 2024
@ivanzlenko ivanzlenko deleted the HDDS-10503 branch March 22, 2024 05:25
myskov pushed a commit to myskov/ozone that referenced this pull request Apr 3, 2024
myskov pushed a commit to myskov/ozone that referenced this pull request Apr 3, 2024
myskov pushed a commit to myskov/ozone that referenced this pull request Apr 4, 2024
myskov pushed a commit to myskov/ozone that referenced this pull request Apr 4, 2024
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.

3 participants