pvary commented on a change in pull request #1394:
URL: https://github.com/apache/iceberg/pull/1394#discussion_r480924004
##########
File path: hive/src/main/java/org/apache/iceberg/hive/HiveCatalog.java
##########
@@ -300,17 +300,11 @@ public boolean setProperties(Namespace namespace,
Map<String, String> propertie
parameter.putAll(properties);
Database database = convertToDatabase(namespace, parameter);
- boolean result = alterHiveDataBase(namespace, database);
+ alterHiveDataBase(namespace, database);
+ LOG.debug("Successfully set properties for {}", namespace);
Review comment:
> > This way we will at least know how the
SparkCatalog.alterNamespace/FlinkCatalog.alterNamespace classified the
properties, and which one was removed which one was set instead.
>
> I don't think I follow your logic here. All this says is that
`setProperties` succeeded. But that's usually obvious from the job because the
SQL that's running is `ALTER TABLE ... SET TBLPROPERTIES` or `ALTER TABLE ...
UNSET TBLPROPERTIES`. So it seems to me that this is already clear from context.
I understand your concerns about logging where the only added info we have
is that the code line is reached, and I agree that is rarely useful (I
occasionally find them handy when debugging performance issues, but that is a
rare case). This is why I have removed the logs in the first commit.
After pushing the commit I checked again how the catalog calls are called.
In the fear of stating the obvious:
- SparkCatalog.alterNamespace/FlinkCatalog.alterNamespace gets an array of
NamespaceChange and splits them to 2 lists updates/removals.
- Updates ends in setProperties call, removals ends in a removeProperties
call.
That is why I have added back and modified the setProperties logging, so it
will log not only the call but also logs the *properties.keySet()* too. This
way we could know which properties were removed, and which were updated in this
call. I think this could be beneficial to have in debug level, unless this info
is also available somewhere else in the logs.
> > I kinda feel that I have dragged on too much on your patience, so one
last request and I will remove both log lines from the code.
>
> Don't worry about it! I had a similar thought because I'm asking for so
many logs to be removed or changed. Sounds like we're both happy to come to an
agreement and improve logging, even if that takes a lot of discussion. Thanks
for your patience here.
Good to meet a likeminded person, and thanks for all the useful help here!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
