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.

Sign up for GitHub

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

Jump to bottom

#25: create class for partitions #45

Merged

benedeki merged 19 commits into master from feature/25create-class-4-segmentation-2

Sep 18, 2023

Contributor

seldon851 commented Jun 28, 2023 •

edited by benedeki

Loading

It's now possible to create an AtumContext of sub-partitions
AtumAgent now a class and companion object of the class to allow referencing
AtumContext now behaves somewhat like a singleton for given AtumPartitions - as a result it's mutable
Rename Measurement to Measure
Simplified AtumPartitions

Closes #25
Closes #33

Hector Perez added 3 commits

June 28, 2023 12:48


          create AtumPartitions.scala

51fa957


          minor

2918fd0


          format

ce4c6e8

seldon851 requested a review from benedeki

June 28, 2023 11:55

Hector Perez added 3 commits

June 28, 2023 12:57


          comment

507b03b


          naming

f15d728


          add some tests

ddce673

benedeki reviewed

View reviewed changes

agent/src/main/scala/za/co/absa/atum/agent/model/AtumPartitions.scala Outdated

+              import java.time.LocalDate
+              import scala.collection.immutable.ListMap
+              case class AtumPartitions(name: String, reportDate: LocalDate, partitions: Partitions = ListMap()) {

Contributor

benedeki Jun 29, 2023

name and reportDate are surplus. There are no specific keys always present, so these while to be expected to be common, should be part of the partitons key.

agent/src/main/scala/za/co/absa/atum/agent/model/AtumPartitions.scala Outdated

+              import java.time.LocalDate
+              import scala.collection.immutable.ListMap
+              case class AtumPartitions(name: String, reportDate: LocalDate, partitions: Partitions = ListMap()) {

Contributor

benedeki Jun 29, 2023

This class is kind of a key toe AtumContext definitely should be a member of the class.

agent/src/main/scala/za/co/absa/atum/agent/AtumAgent.scala Outdated

+                 *  @return
+                 */
+                def createAtumContext(atumPartitions: AtumPartitions): AtumContext = {
+                  AtumContext()

Contributor

benedeki Jun 29, 2023

This would need more logic. According to my insight, the returned value should be a kind of singleton of AtumContext. For same atumPartitions the same AtumContext needs to be returned.
From that POV I am even considering making AtumContext mutable in the regard of Measures. What do you think guys? @seldon851, @Zejnilovic, @dk1844 , @lsulak, @jakipatryk, @yruslan

Hector Perez added 5 commits

June 29, 2023 13:35


          some CH req

8130c4a


          comment

b2cdd2f


          minor

48dd958


          README.md

c9e43cc


          minor

bbfe10b

seldon851 requested a review from benedeki

June 30, 2023 12:58


          minor

f6df867

seldon851 marked this pull request as ready for review

June 30, 2023 14:19


          post review fixes, just minor ones

47d92c8

lsulak requested review from lsulak, TebaleloS, Zejnilovic and dk1844 as code owners

August 21, 2023 14:11

github-actions bot commented Aug 21, 2023 •

edited

Loading

JaCoCo agent module code coverage report - spark:2 - scala 2.12.12

File	Coverage [66.28%]	❌
MeasureResult.scala	100%	🍏
AtumAgent.scala	81.82%	🍏
AtumContext.scala	76.03%	❌
Measure.scala	54.61%	❌

Total Project Coverage	65.64%	❌

github-actions bot commented Aug 21, 2023

JaCoCo server module code coverage report - scala 2.12.12

There is no coverage information present for the Files changed

Total Project Coverage	12.45%	❌


          #25: post review fixes, adding missing licences to Scala files

0f3099d

TebaleloS reviewed

View reviewed changes

agent/src/main/scala/za/co/absa/atum/agent/AtumAgent.scala Outdated

+                )
+                /**
+                 *  Sends a single `MeasureResult` to the AtumService API. With not AtumContext involve.

Collaborator

TebaleloS Aug 22, 2023

Suggested change

      
               *  Sends a single `MeasureResult` to the AtumService API. With not AtumContext involve.
          
               *  Sends a single `MeasureResult` to the AtumService API. Without AtumContext involve.

lsulak and others added 2 commits

August 22, 2023 12:19


          #25: just making it more English and consistent

5a2b61b


          * It's now possible to create an AtumContext of sub-partitions

29f8300

* `AtumAgent` now a class and companion object of the class to allow referencing
* `AtumContext` now behaves somewhat like a singleton for given `AtumPartitions` - as a result it's mutable
* Rename `Measurement` to `Measure`
* Simplified `AtumPartitions`

lsulak reviewed

View reviewed changes

agent/src/main/scala/za/co/absa/atum/agent/AtumContext.scala Outdated Show resolved Hide resolved

lsulak reviewed

View reviewed changes

Collaborator

lsulak left a comment

LGTM so far, no big issues, just minor

agent/src/main/scala/za/co/absa/atum/agent/model/MeasureResult.scala Show resolved Hide resolved

agent/src/main/scala/za/co/absa/atum/agent/model/AtumPartitions.scala Outdated Show resolved Hide resolved

agent/src/main/scala/za/co/absa/atum/agent/model/AtumPartitions.scala Outdated Show resolved Hide resolved

agent/src/main/scala/za/co/absa/atum/agent/model/AtumPartitions.scala Outdated Show resolved Hide resolved

agent/src/main/scala/za/co/absa/atum/agent/model/AtumPartitions.scala Outdated Show resolved Hide resolved

agent/src/main/scala/za/co/absa/atum/agent/model/AtumPartitions.scala Outdated Show resolved Hide resolved

agent/src/main/scala/za/co/absa/atum/agent/model/AtumPartitions.scala Outdated Show resolved Hide resolved

agent/src/main/scala/za/co/absa/atum/agent/model/AtumPartitions.scala Outdated Show resolved Hide resolved

agent/src/main/scala/za/co/absa/atum/agent/AtumContext.scala Outdated Show resolved Hide resolved


          * removed references to non-committed code

5b9b422

* deleted obsolete class


          Merge remote-tracking branch 'remotes/origin/master' into feature/25c…

a074610

…reate-class-4-segmentation-2

lsulak reviewed

View reviewed changes

agent/src/main/scala/za/co/absa/atum/agent/AtumContext.scala Outdated

                       val result = MeasureResult(measure, measure.function(df))
                       AtumAgent.publish(checkpointName, atumContext, result)
-                      executeMeasures(checkpointName, atumContext.measurements)
+                      executeMeasures(checkpointName, atumContext.measures)

Collaborator

lsulak Sep 15, 2023

this line can be removed, it's doing the same as the whole foreach basically

Contributor

benedeki Sep 18, 2023

Will removed, but as said, this needs reword. createCheckpoint and send it as an "atomic" steo.

lsulak reviewed

View reviewed changes

agent/src/main/scala/za/co/absa/atum/agent/AtumContext.scala

                    *  @param measure the measure to be calculated
                    *  @return
                    */
-                  def executeMeasure(checkpointName: String, measure: Measurement): DataFrame = {
+                  def executeMeasure(checkpointName: String, measure: Measure): DataFrame = {

Collaborator

lsulak Sep 15, 2023

I think that execute should do only executing and publishing should do publishing. And it can be all put together in some other operation.

I definitely wouldn't expect from 'executeMeasure' to actually also send data to the server

Contributor

benedeki Sep 18, 2023

I think these could and should be eventually completely removed. Thee;s no scenario where just particular measure are recorded for a checkpoint.

lsulak reviewed

View reviewed changes

agent/src/main/scala/za/co/absa/atum/agent/AtumContext.scala

@@ @@ -78,11 +109,11 @@ object AtumContext { @@
                    *  @return
                    */
                   def createCheckpoint(checkpointName: String)(implicit atumContext: AtumContext): DataFrame = {
-                    atumContext.measurements.foreach { measure =>
+                    atumContext.measures.foreach { measure =>
                       val result = MeasureResult(measure, measure.function(df))

Collaborator

lsulak Sep 15, 2023

here you can call executeMeasure function.

Contributor

benedeki Sep 18, 2023

As it will (probably) go away, can this stay? 😉


          * addressing PR comments

7faadcc

lsulak approved these changes

View reviewed changes

Collaborator

lsulak left a comment

Approved. Obviously the code is a bit chaotic with the current implementation versus what we want to achieve - we can build something on top of this, but rework a lot. Let's merge it and deal with it later (but soon)

benedeki merged commit aec3c76 into master

4 of 5 checks passed

benedeki deleted the feature/25create-class-4-segmentation-2 branch

September 18, 2023 16:04

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

Reviewers

TebaleloS TebaleloS left review comments

lsulak lsulak approved these changes

benedeki Awaiting requested review from benedeki benedeki is a code owner

Zejnilovic Awaiting requested review from Zejnilovic Zejnilovic is a code owner

dk1844 Awaiting requested review from dk1844 dk1844 is a code owner

Labels

None yet