feat(ingestion/iceberg): Several improvements to iceberg connector

[skrydal]

• Extend datasetProperties information with creation and last modified times (if available)
• Handle FileIO errors (class not found) as warnings - extracted from ValueError exception, in long term we should probably change pyiceberg library to throw unique exception type in such case
• Timings reporting classes were not thread-safe, introduced locks
• Extend datasetProperties with qualifiedName (set as namespace.table)

[codecov]

Codecov Report

Attention: Patch coverage is 93.02326% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...on/src/datahub/ingestion/source/iceberg/iceberg.py	91.66%	2 Missing ⚠️
...datahub/ingestion/source/iceberg/iceberg_common.py	94.73%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[sgomezvillamor]

Tuple[str, ...] seems equivalent to List[str]

I guess only difference is the tuple ensures a size >= 1 and that's relevant here, right?

[skrydal]

There are more differences between a tuple and a list and this annotation reflects that. For one, tuple is immutable and passed by value. Moreover this annotation matches what we are getting from pyiceberg, so overall I would like to keep it like this.

[sgomezvillamor]

may you have forgot the redundant LOGGER.warning for this except case?

[skrydal]

warning method will automatically log, unlike report_warning. I should change all lines to use warning I think, see:

datahub/metadata-ingestion/src/datahub/ingestion/api/source.py

Lines 247 to 267 in 5309ae0

	def report_warning(
	self,
	message: LiteralString,
	context: Optional[str] = None,
	title: Optional[LiteralString] = None,
	exc: Optional[BaseException] = None,
	) -> None:
	self._structured_logs.report_log(
	StructuredLogLevel.WARN, message, title, context, exc, log=False
	)
	def warning(
	self,
	message: LiteralString,
	context: Optional[str] = None,
	title: Optional[LiteralString] = None,
	exc: Optional[BaseException] = None,
	) -> None:
	self._structured_logs.report_log(
	StructuredLogLevel.WARN, message, title, context, exc, log=True
	)

[sgomezvillamor]

LGTM

Adding locks feels repetitive code.
Not a big fan of decorators, but they may be helpful here.

[skrydal]

LGTM

Adding locks feels repetitive code. Not a big fan of decorators, but they may be helpful here.

Considering they are singe lines with lock... I think decorators will take same amount of lines (beside lock initialization), right? Do you have any particular decorator in your mind which I could use?

[sgomezvillamor]

Do you have any particular decorator in your mind which I could use?

Unexpectedly, I haven't found any! 😅