You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/06/04 07:23:14 UTC
[GitHub] [iceberg] kbendick opened a new pull request #2674: [Hotfix] - Minor typo in error message in test
kbendick opened a new pull request #2674:
URL: https://github.com/apache/iceberg/pull/2674
I came across this minor typo working on a separate issue.
I'd include it in that work to reduce commits, but ultimately I won't need to touch this file in that other PR.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2674: [Hotfix] - Minor typo in error message in spark 2 TestCatalog
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2674:
URL: https://github.com/apache/iceberg/pull/2674#discussion_r645350924
##########
File path: spark2/src/test/java/org/apache/iceberg/spark/source/TestCatalog.java
##########
@@ -95,7 +95,7 @@ public void initialize(String name, Map<String, String> properties) {
String uri = properties.get(CatalogProperties.URI);
warehouse = properties.get("warehouse");
Preconditions.checkArgument(uri != null, "A uri parameter must be set");
- Preconditions.checkArgument(uri.contains("thrift"), "A ur parameter must be valid");
+ Preconditions.checkArgument(uri.contains("thrift"), "A uri parameter must be valid");
Review comment:
While we're here, should we consider changing this to something more explanatory?
Perhaps `Catalog uri parameter must be of type thrift` would be better?
I don't have a strong opinion either way, since it's just a test. But given that I'm here already, figured I'd bring it up. I strongly prefer informative error messages over more vague error messages.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #2674: [Hotfix] - Minor typo in error message in spark 2 TestCatalog
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #2674:
URL: https://github.com/apache/iceberg/pull/2674#issuecomment-854546313
> Would love to chat about that stuff. I keep on promising myself I will make time to work on it too :-)
>
> cc @nastra and @jasonhughes248 they have also been looking at some community stuff and may be interested
Cool. I basically have a pretty good working starting point, so I will cut a (possibly new) ticket and push that by Monday or Tuesday and we can iterate off of that or use it as a starting point of discussion.
I've also got a handful of helper scripts that might be overkill. I just need to remove some of the internal stuff (which is probably going to be as simple as updating the base docker image). Looking forward to it.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr merged pull request #2674: Clarify preconditions in spark 2 TestCatalog initialization
Posted by GitBox <gi...@apache.org>.
rymurr merged pull request #2674:
URL: https://github.com/apache/iceberg/pull/2674
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2674: [Hotfix] - Minor typo in error message in spark 2 TestCatalog
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2674:
URL: https://github.com/apache/iceberg/pull/2674#discussion_r645350924
##########
File path: spark2/src/test/java/org/apache/iceberg/spark/source/TestCatalog.java
##########
@@ -95,7 +95,7 @@ public void initialize(String name, Map<String, String> properties) {
String uri = properties.get(CatalogProperties.URI);
warehouse = properties.get("warehouse");
Preconditions.checkArgument(uri != null, "A uri parameter must be set");
- Preconditions.checkArgument(uri.contains("thrift"), "A ur parameter must be valid");
+ Preconditions.checkArgument(uri.contains("thrift"), "A uri parameter must be valid");
Review comment:
While we're here, should we consider changing this to something more explanatory?
Perhaps `Catalog uri parameter must be of type thrift` would be better?
I don't have a strong opinion either way, since it's just a test. But given that I'm here already, figured I'd bring it up. I strongly prefer informative error messages over more vague error messages.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2674: [Hotfix] - Minor typo in error message in spark 2 TestCatalog
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2674:
URL: https://github.com/apache/iceberg/pull/2674#discussion_r645437726
##########
File path: spark2/src/test/java/org/apache/iceberg/spark/source/TestCatalog.java
##########
@@ -95,7 +95,7 @@ public void initialize(String name, Map<String, String> properties) {
String uri = properties.get(CatalogProperties.URI);
warehouse = properties.get("warehouse");
Preconditions.checkArgument(uri != null, "A uri parameter must be set");
- Preconditions.checkArgument(uri.contains("thrift"), "A ur parameter must be valid");
+ Preconditions.checkArgument(uri.contains("thrift"), "A uri parameter must be valid");
Review comment:
Always happy to help make the project more friendly to newcomers 👍.
I'll update the Preconditions in the morning (it's admittedly really later where I am).
I've got some other, much larger and long overdue community help stuff lined up too (a docker-compose environment) that I am hoping to push next week. I'd love to get it working first for the basics (spark with a consistent metastore - currently barebones Hive with HDFS) and then add Nessie to it as well 🙂. I'm currently in the process of removing the internal bits and then deciding on what is too much, but will probably just start small and iterate. I'll tag you there and might reach out to ask for your opinion as well if you don't mind.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2674: [Hotfix] - Minor typo in error message in spark 2 TestCatalog
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2674:
URL: https://github.com/apache/iceberg/pull/2674#discussion_r645437726
##########
File path: spark2/src/test/java/org/apache/iceberg/spark/source/TestCatalog.java
##########
@@ -95,7 +95,7 @@ public void initialize(String name, Map<String, String> properties) {
String uri = properties.get(CatalogProperties.URI);
warehouse = properties.get("warehouse");
Preconditions.checkArgument(uri != null, "A uri parameter must be set");
- Preconditions.checkArgument(uri.contains("thrift"), "A ur parameter must be valid");
+ Preconditions.checkArgument(uri.contains("thrift"), "A uri parameter must be valid");
Review comment:
Always happy to help make the project more friendly to newcomers 👍.
I'll update the Preconditions in the morning (it's admittedly really later where I am).
I've got some other, much larger and long overdue community help stuff lined up too (a docker-compose environment) that I am hoping to push next week. I'd love to get it working first for the basics (spark with a consistent metastore - currently barebones Hive) and then add Nessie to it as well 🙂. I'm currently in the process of removing the internal bits and then deciding on what is too much, but will probably just start small and iterate. I'll tag you there and might reach out to ask for your opinion as well if you don't mind.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #2674: Clarify preconditions in spark 2 TestCatalog initialization
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #2674:
URL: https://github.com/apache/iceberg/pull/2674#issuecomment-854582461
> for what its worth we have:
>
> * A [Demo notebook](https://colab.research.google.com/github/projectnessie/nessie-demos/blob/main/colab/nessie-iceberg-demo-nba.ipynb) which sets up Nessie, Spark etc and runs some basic iceberg commands
> * A [Spark Config notebook](https://colab.research.google.com/github/projectnessie/nessie-demos/blob/main/colab/nessie-iceberg-spark-setup.ipynb) to hihglight the important configs for spark when running iceberg
> * a [docker-compose.yaml](https://github.com/projectnessie/nessie/blob/main/python/demo/docker-compose.yml) specifically for iceberg/spark.
>
> Given the nessie specifics it might not be exactly right for you but hopefully there are some bits that could be reusable. We also did all of our demos/examples in python so that they could run on colab w/o extra infrastructure
Cool. That will likely complement well with what I have, though it might need some tweaking. I'm opening a ticket now, but I will try to get my stuff up early next week for review and then we can iterate from there? We don't necessarily have to use my setup even, but it's probably the lowest common denominator while still being fully featured. The one big thing it's missing currently is a notebook server, which I was going to add, but in the interest of getting it up for discussion and community review, I'll just go with the basics. cc'ing you and the folks you mentioned on the ticket now!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr commented on pull request #2674: [Hotfix] - Minor typo in error message in spark 2 TestCatalog
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #2674:
URL: https://github.com/apache/iceberg/pull/2674#issuecomment-854541331
Would love to chat about that stuff. I keep on promising myself I will make time to work on it too :-)
cc @nastra and @jasonhughes248 they have also been looking at some community stuff and may be interested
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2674: [Hotfix] - Minor typo in error message in spark 2 TestCatalog
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2674:
URL: https://github.com/apache/iceberg/pull/2674#discussion_r645431837
##########
File path: spark2/src/test/java/org/apache/iceberg/spark/source/TestCatalog.java
##########
@@ -95,7 +95,7 @@ public void initialize(String name, Map<String, String> properties) {
String uri = properties.get(CatalogProperties.URI);
warehouse = properties.get("warehouse");
Preconditions.checkArgument(uri != null, "A uri parameter must be set");
- Preconditions.checkArgument(uri.contains("thrift"), "A ur parameter must be valid");
+ Preconditions.checkArgument(uri.contains("thrift"), "A uri parameter must be valid");
Review comment:
Strongly agree, @rymurr.
How about `Cannot initialize TestCatalog, the catalog's metastore connection uri must use thrift as the scheme.`?
The optional fix, in theory, would be something along the lines of `Please update the metastore URI configuration on the SparkContext to a valid Hive metastore thrift URI of the form of 'thrift://host:port', via the key 'spark.sql.catalog.${catalog_name}.uri'`, though in this case, I think that the optional how to fix is likely unnecessary (as it's just a mock, though that is how one would resolve that issue).
What do you think? I'm tempted to update all of these Preconditions while I'm here, as I feel like people often look at the tests to learn how to do things (even if they could realistically get that information from the docs). 🙂
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2674: [Hotfix] - Minor typo in error message in spark 2 TestCatalog
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2674:
URL: https://github.com/apache/iceberg/pull/2674#discussion_r645431837
##########
File path: spark2/src/test/java/org/apache/iceberg/spark/source/TestCatalog.java
##########
@@ -95,7 +95,7 @@ public void initialize(String name, Map<String, String> properties) {
String uri = properties.get(CatalogProperties.URI);
warehouse = properties.get("warehouse");
Preconditions.checkArgument(uri != null, "A uri parameter must be set");
- Preconditions.checkArgument(uri.contains("thrift"), "A ur parameter must be valid");
+ Preconditions.checkArgument(uri.contains("thrift"), "A uri parameter must be valid");
Review comment:
How about `Cannot initialize TestCatalog, the catalog's metastore connection uri must use thrift as the scheme.`?
The optional fix, in theory, would be something along the lines of `Please update the configuration on the SparkContext for 'spark.sql.catalog.${catalog_name}.uri' to the form of 'thrift://host:port'`, though in this case, I think that the optional how to fix is likely unnecessary (as it's just a mock, though that is how one would resolve that issue).
What do you think? I'm tempted to update all of these Preconditions while I'm here, as I feel like people often look at the tests to learn how to do things (even if they could realistically get that information from the docs). 🙂
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr commented on a change in pull request #2674: [Hotfix] - Minor typo in error message in spark 2 TestCatalog
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #2674:
URL: https://github.com/apache/iceberg/pull/2674#discussion_r645433475
##########
File path: spark2/src/test/java/org/apache/iceberg/spark/source/TestCatalog.java
##########
@@ -95,7 +95,7 @@ public void initialize(String name, Map<String, String> properties) {
String uri = properties.get(CatalogProperties.URI);
warehouse = properties.get("warehouse");
Preconditions.checkArgument(uri != null, "A uri parameter must be set");
- Preconditions.checkArgument(uri.contains("thrift"), "A ur parameter must be valid");
+ Preconditions.checkArgument(uri.contains("thrift"), "A uri parameter must be valid");
Review comment:
> How about `Cannot initialize TestCatalog, the catalog's metastore connection uri must use thrift as the scheme.`?
agreed, the fix is probably redundant for the tests
> I'm tempted to update all of these Preconditions while I'm here,
Love the idea, thanks for taking that on. makes a massive difference to the community imho. I always start w/ reading the tests :-)
##########
File path: spark2/src/test/java/org/apache/iceberg/spark/source/TestCatalog.java
##########
@@ -95,7 +95,7 @@ public void initialize(String name, Map<String, String> properties) {
String uri = properties.get(CatalogProperties.URI);
warehouse = properties.get("warehouse");
Preconditions.checkArgument(uri != null, "A uri parameter must be set");
- Preconditions.checkArgument(uri.contains("thrift"), "A ur parameter must be valid");
+ Preconditions.checkArgument(uri.contains("thrift"), "A uri parameter must be valid");
Review comment:
> How about `Cannot initialize TestCatalog, the catalog's metastore connection uri must use thrift as the scheme.`?
agreed, the fix is probably redundant for the tests
> I'm tempted to update all of these Preconditions while I'm here,
Love the idea, thanks for taking that on. makes a massive difference to the community imho. I always start w/ reading the tests :-)
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr commented on pull request #2674: Clarify preconditions in spark 2 TestCatalog initialization
Posted by GitBox <gi...@apache.org>.
rymurr commented on pull request #2674:
URL: https://github.com/apache/iceberg/pull/2674#issuecomment-854568624
for what its worth we have:
* A [Demo notebook](https://colab.research.google.com/github/projectnessie/nessie-demos/blob/main/colab/nessie-iceberg-demo-nba.ipynb) which sets up Nessie, Spark etc and runs some basic iceberg commands
* A [Spark Config notebook](https://colab.research.google.com/github/projectnessie/nessie-demos/blob/main/colab/nessie-iceberg-spark-setup.ipynb) to hihglight the important configs for spark when running iceberg
* a [docker-compose.yaml](https://github.com/projectnessie/nessie/blob/main/python/demo/docker-compose.yml) specifically for iceberg/spark.
Given the nessie specifics it might not be exactly right for you but hopefully there are some bits that could be reusable. We also did all of our demos/examples in python so that they could run on colab w/o extra infrastructure
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rymurr commented on a change in pull request #2674: [Hotfix] - Minor typo in error message in spark 2 TestCatalog
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #2674:
URL: https://github.com/apache/iceberg/pull/2674#discussion_r645396043
##########
File path: spark2/src/test/java/org/apache/iceberg/spark/source/TestCatalog.java
##########
@@ -95,7 +95,7 @@ public void initialize(String name, Map<String, String> properties) {
String uri = properties.get(CatalogProperties.URI);
warehouse = properties.get("warehouse");
Preconditions.checkArgument(uri != null, "A uri parameter must be set");
- Preconditions.checkArgument(uri.contains("thrift"), "A ur parameter must be valid");
+ Preconditions.checkArgument(uri.contains("thrift"), "A uri parameter must be valid");
Review comment:
I agree @kbendick a more informative message would be better. @rdblue always asks for 'cannot do X, reason. Optionally how to fix'. I think that's a great standard for the project, even in tests.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #2674: [Hotfix] - Minor typo in error message in spark 2 TestCatalog
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2674:
URL: https://github.com/apache/iceberg/pull/2674#discussion_r645431837
##########
File path: spark2/src/test/java/org/apache/iceberg/spark/source/TestCatalog.java
##########
@@ -95,7 +95,7 @@ public void initialize(String name, Map<String, String> properties) {
String uri = properties.get(CatalogProperties.URI);
warehouse = properties.get("warehouse");
Preconditions.checkArgument(uri != null, "A uri parameter must be set");
- Preconditions.checkArgument(uri.contains("thrift"), "A ur parameter must be valid");
+ Preconditions.checkArgument(uri.contains("thrift"), "A uri parameter must be valid");
Review comment:
Strongly agree, @rymurr.
How about `Cannot initialize TestCatalog, the catalog's metastore connection uri must use thrift as the scheme.`?
The optional fix, in theory, would be something along the lines of `Please update the configuration on the SparkContext for 'spark.sql.catalog.${catalog_name}.uri' to the form of 'thrift://host:port'`, though in this case, I think that the optional how to fix is likely unnecessary (as it's just a mock, though that is how one would resolve that issue).
What do you think? I'm tempted to update all of these Preconditions while I'm here, as I feel like people often look at the tests to learn how to do things (even if they could realistically get that information from the docs). 🙂
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org