Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Improve Authentication Error handling #21087
Improve Authentication Error handling #21087
Changes from 5 commits
2bc0f2c
2de7385
f003856
8130a40
66a8201
d684daf
fc66209
7beef18
ca10a01
6f8e578
88c3518
de1a587
3387d09
e0fd67b
fb60113
4437101
9bb61bb
23ef1c1
a7a0a8f
10c67fd
15e7e76
e6306bc
8b6e557
4fd4537
06788f0
6403184
c071fc5
dd9b783
18ea9ec
f76ba9b
7c2ff54
98042f0
7132633
320ed3f
b7e2948
44ebed3
836e2e5
0fc73d8
8b61e77
0ab8432
4f14b79
972794a
c779c49
0cc751e
961aae0
44daeb5
ad140b5
6becd86
43baa05
98da7ae
00780e9
9a9ca55
ecdd732
252333c
5360239
c83b293
afb63cc
3673aab
de8a177
4c60cb5
07c4fbf
cd00b48
77f463d
81947aa
a93ac03
fa548e9
85c042c
2b11350
14b618f
edbde3b
d780fe9
c3d842f
b68d130
7e0b526
af5faad
2ae3d51
c83ffff
53557c3
1a25de2
e42355b
6a336fe
4a2a5eb
de748b8
31e1860
29e2a30
66a475d
167e528
18c1776
a8a10bb
26a556d
9918386
b911717
be86bd5
4821113
3437500
e15bb43
68f3858
43e3a24
0084339
94021fa
77b08a4
76585a9
307c499
b578675
9df3e0a
a2bc9e4
b280df2
9c32277
64702d7
2de2bb6
5bdc39c
c97685c
45c2f20
cf876b1
9ba1fce
6eaec8b
026e857
3a4ac04
a7089da
a558b8d
b336d6c
7d3d837
f715310
021b715
db25e3c
507a90a
8063c35
1024e3f
8b943a7
a79fe8c
bcad152
1f0c569
4d5ed05
a060797
3ac31da
8b65da1
1441218
974915e
f956265
c1f1175
f467803
3396fce
c8f0e1c
16c2f2c
30d49c0
7900e2c
43109da
e91dc6c
bced390
2196c7a
e68b907
fc1bdc8
c3a2e63
218b42a
2256412
6aa54ac
d174dd0
beae36c
eb07473
3646cda
6f3fcc8
c9e254f
6783d58
18bf629
0e4a808
50b38ba
046f3fb
79e0245
2f91349
c4d9892
4302b67
5c6f5b4
159ca81
5c70686
680f211
5177665
9244363
a22e2a3
fd1537e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale behind checking that all exceptions are authentication issues?
Similar to
BigQueryGcsOperations
can this Auth Error Code be a constant so anybody not sure how come403
can easily understand the value behind the integer?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on making it a constant
Rationale for checking that all errors are auth errors is that theoretically there could be a transient issue with permissions thats resolved on a retry. All issues being auth issues was the behavior I observed when permissions were not set correctly and it seemed safer to make a judgement call on this being a configuration issue when this occurs since matching
any
as opposed toall
might hide a different issueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, that makes sense. I would add that in as a comment if you can since I can see someone looking at this code and coming with a different conclusion and just checking if
any
of the exceptions are Auth and calling it a dayFurther more in the
BigQueryException
class they have a section that goes likeMight be worth considering naming the errors as
AUTH_ERRORS
and placing them into some general utility file likeJdbcUtils.java
so they don't need to be recreated across each destination connectorThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I'm reading this correctly, this currently checks that all
AmazonS3Exception
are auth errors, but ignores any non-amazon exceptions. E.g. if there's a randomIllegalStateException
in this list, it would be treated as though it's an auth errorI think it might be better to bundle the filter + map together?
(womp, didn't see greg's comment before posting this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me awhile but I think I see what you and @grishick are saying - updating this to ensure that really all of the errors are auth errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also formatting nit, I'd prefer to have the
allMatch
on a newline to make this easier to read, i.e.(I find it hard to keep track of which parentheses surround which expressions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on the multiline styling, I wasn't sure what the preference here was so I let the autoformatter decide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit: since we're only catching one exception type, it doesn't seem necessary to have an Exception value that differs from the rest of the code
That said, it's not blocking. It's mostly to keep consistency across our code base.
Also in the super class of
BigQueryException
,BaseServiceException
there's a methodgetReason()
that might be more useful to return back to the user for how come something caused this exceptionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly not sure how this happened, I have a blank new line at the end of my file and it says its up to date.