Skip to content

Use error message type from spark commons: 2170 #2177

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
Open
TebaleloS wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Use error message type from spark commons: 2170 #2177

TebaleloS wants to merge 8 commits into from

Conversation

@TebaleloS
Copy link
Collaborator

@TebaleloS TebaleloS commented Feb 24, 2023

Changes

  • Upgraded the spark-common version from 0.4.0 to 0.5.0
  • Removed ErrorMessage case class
  • Removed Mapping case class
  • Removed errorColumnName property
  • Renamed Enceladus ErrorMessage file and the Object to EnceladusErrorMessage
  • Replaced Enceladus ErrorMerssage with spark-common ErrorMessages
  • Closes Use ErrorMessage type from spark-commons #2170

@TebaleloS TebaleloS marked this pull request as ready for review February 24, 2023 16:07
benedeki
benedeki requested changes Feb 24, 2023
View reviewed changes
Copy link
Collaborator

@benedeki benedeki left a comment

Choose a reason for hiding this comment

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

Seems class ErrorMessageFactory can be safely deleted too.

Does not build 😞 Several files still refer to old classes.

...n/scala/za/co/absa/enceladus/plugins/builtin/errorsender/mq/KafkaErrorSenderPluginImpl.scala
import za.co.absa.abris.avro.functions.to_avro
import za.co.absa.abris.config.{AbrisConfig, ToAvroConfig}
import za.co.absa.enceladus.plugins.builtin.errorsender.mq.kafka.KafkaErrorSenderPlugin.{avroKeySchemaRegistryConfig, avroValueSchemaRegistryConfig, registerSchemas}
import za.co.absa.enceladus.utils.error.EnceladusErrorMessage.ErrorCodes
Copy link
Collaborator

@benedeki benedeki Feb 24, 2023

Choose a reason for hiding this comment

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

Minor:
Some unused imports.

spark-jobs/src/main/scala/za/co/absa/enceladus/conformance/interpreter/DynamicInterpreter.scala Outdated
import za.co.absa.enceladus.conformance.interpreter.rules.custom.CustomConformanceRule
import za.co.absa.enceladus.conformance.interpreter.rules.mapping.{MappingRuleInterpreter, MappingRuleInterpreterBroadcast,
MappingRuleInterpreterGroupExplode}
import za.co.absa.enceladus.conformance.interpreter.rules.mapping.{MappingRuleInterpreter, MappingRuleInterpreterBroadcast, MappingRuleInterpreterGroupExplode}
Copy link
Collaborator

@benedeki benedeki Feb 24, 2023

Choose a reason for hiding this comment

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

Really minor:
Line is over 140 character long.

Copy link
Collaborator Author

@TebaleloS TebaleloS Mar 2, 2023

Choose a reason for hiding this comment

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

I'm not sure about what I should do here, or should I just import everything from maping e.g. za.co.absa.enceladus.conformance.interpreter.rules.mapping._?

Copy link
Collaborator

@lsulak lsulak Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I believe that multi-line imports should work:

import za.co.absa.enceladus.conformance.interpreter.rules.mapping.{
  MappingRuleInterpreter, MappingRuleInterpreterBroadcast, MappingRuleInterpreterGroupExplode
}

that can be one way how to resolve this

Copy link
Collaborator Author

@TebaleloS TebaleloS Mar 2, 2023

Choose a reason for hiding this comment

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

Yeah it does work, but gives a warning. But I have refactored it now.

...bsa/enceladus/conformance/interpreter/rules/mapping/MappingRuleInterpreterGroupExplode.scala Outdated
import za.co.absa.enceladus.dao.EnceladusDAO
import za.co.absa.enceladus.model.conformanceRule.{ConformanceRule, MappingConformanceRule}
import za.co.absa.enceladus.model.{Dataset => ConfDataset}
import za.co.absa.enceladus.utils.error._
Copy link
Collaborator

@benedeki benedeki Feb 24, 2023

Choose a reason for hiding this comment

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

Not used.

lsulak
lsulak reviewed Feb 27, 2023
View reviewed changes
Copy link
Collaborator

@lsulak lsulak left a comment

Choose a reason for hiding this comment

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

The changes looks good to me, but the code still won't compile because there are some places where it fails on wrong imports, as David said before.

Once it's fixed, I can quickly test it.

lsulak
lsulak reviewed Mar 2, 2023
View reviewed changes
...n/scala/za/co/absa/enceladus/plugins/builtin/errorsender/mq/KafkaErrorSenderPluginImpl.scala
import za.co.absa.abris.avro.functions.to_avro
import za.co.absa.abris.config.{AbrisConfig, ToAvroConfig}
import za.co.absa.abris.config.{ToAvroConfig}
import za.co.absa.enceladus.plugins.builtin.errorsender.mq.kafka.KafkaErrorSenderPlugin.{avroKeySchemaRegistryConfig, avroValueSchemaRegistryConfig, registerSchemas}
Copy link
Collaborator

@lsulak lsulak Mar 2, 2023

Choose a reason for hiding this comment

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

this line has 166 chars

lsulak
lsulak reviewed Mar 2, 2023
View reviewed changes
spark-jobs/src/test/scala/za/co/absa/enceladus/standardization/StandardizationRerunSuite.scala
private val tmpFilePrefix = "test-input-"
private val tmpFileSuffix = ".csv"


Copy link
Collaborator

@lsulak lsulak Mar 2, 2023

Choose a reason for hiding this comment

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

unnecessary empty line

lsulak
lsulak reviewed Mar 2, 2023
View reviewed changes
utils/src/main/scala/za/co/absa/enceladus/utils/error/EnceladusErrorMessage.scala
*/
case class ErrorMessage(errType: String, errCode: String, errMsg: String, errCol: String, rawValues: Seq[String], mappings: Seq[Mapping] = Seq())
case class Mapping(mappingTableColumn: String, mappedDatasetColumn: String)
//case class ErrorMessage(errType: String, errCode: String, errMsg: String, errCol: String, rawValues: Seq[String], mappings: Seq[Mapping] = Seq())
Copy link
Collaborator

@lsulak lsulak Mar 2, 2023

Choose a reason for hiding this comment

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

dead code here and below

lsulak
lsulak reviewed Mar 2, 2023
View reviewed changes
utils/src/test/scala/za/co/absa/enceladus/utils/broadcast/BroadcastUtilsSuite.scala
import org.apache.spark.sql.{DataFrame, Row}
import org.scalatest.wordspec.AnyWordSpec
import za.co.absa.enceladus.utils.error.Mapping
import za.co.absa.enceladus.utils.error.EnceladusErrorMessage
Copy link
Collaborator

@lsulak lsulak Mar 2, 2023

Choose a reason for hiding this comment

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

this is not used

Copy link
Collaborator Author

@TebaleloS TebaleloS Mar 3, 2023

Choose a reason for hiding this comment

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

I don't have that file in my local branch.

Copy link
Collaborator

@lsulak lsulak Apr 14, 2023

Choose a reason for hiding this comment

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

how's that possible, did you remove it? What git status tells you?

lsulak
lsulak reviewed Mar 2, 2023
View reviewed changes
Copy link
Collaborator

@lsulak lsulak left a comment

Choose a reason for hiding this comment

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

Looks good, I found some minor issues, most of them just formatting.

  • code reviewed
  • pulled
  • built
  • ran tests

@TebaleloS @lsulak
Update plugins-builtin/src/main/scala/za/co/absa/enceladus/plugins/bu…
e2e7ddb 
…iltin/errorsender/mq/KafkaErrorSenderPluginImpl.scala

Co-authored-by: Ladislav Sulak <laco.sulak@gmail.com>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@benedeki benedeki added under discussion Requires consideration before a decision is made whether/how to implement work in progress Work on this item is not yet finished (mainly intended for PRs) labels Mar 27, 2023
@benedeki
Copy link
Collaborator

benedeki commented Apr 18, 2023

I am sorry, I am afraid this PR become obsolete with ErrorHandling introduction. I suggest closing it, so as the ticket without merging. And deleting the branch.

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

Reviewers

@lsulak lsulak lsulak left review comments

@benedeki benedeki benedeki requested changes

@dk1844 dk1844 Awaiting requested review from dk1844 dk1844 is a code owner

@jakipatryk jakipatryk Awaiting requested review from jakipatryk jakipatryk is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

under discussion Requires consideration before a decision is made whether/how to implement work in progress Work on this item is not yet finished (mainly intended for PRs)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use ErrorMessage type from spark-commons

4 participants

@TebaleloS @benedeki @lsulak