Skip to content
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

Use HttpJsonRequest instead of HttpJsonHelper #1163

Merged
merged 2 commits into from
May 4, 2016

Conversation

garagatyi
Copy link

@skabashnyuk @sleshchenko @evoevodin Please review

@skabashnyuk
Copy link
Contributor

ok

@skabashnyuk
Copy link
Contributor

Can we remove HttpJsonHelper after that?

@garagatyi
Copy link
Author

Yes, we can!

@@ -95,8 +97,11 @@ public ServiceDescriptor getServiceDescriptor() throws IOException, ServerExcept
myServiceDescriptor = serviceDescriptor;
if (myServiceDescriptor == null) {
try {
myServiceDescriptor = serviceDescriptor = HttpJsonHelper.options(getServiceDescriptorClass(), baseUrl);
} catch (NotFoundException | ConflictException | UnauthorizedException | ForbiddenException e) {
myServiceDescriptor = serviceDescriptor = requestFactory.fromUrl(baseUrl)
Copy link
Member

Choose a reason for hiding this comment

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

WDYT about rewriting this method without myServiceDescriptor variable?

        if (serviceDescriptor == null) {
            synchronized (this) {
                if (serviceDescriptor == null) {
                    try {
                        return serviceDescriptor = requestFactory.fromUrl(baseUrl)
                                                          .useOptionsMethod()
                                                          .request()
                                                          .as(getServiceDescriptorClass(), null);
                    } catch (NotFoundException | ConflictException | UnauthorizedException | ForbiddenException e) {
                        throw new ServerException(e.getServiceError());
                    }
                }
            }
        }

Copy link
Author

Choose a reason for hiding this comment

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

Looks OK for me from concurrency POW. @evoevodin WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@sleshchenko
Copy link
Member

LGTM

@voievodin
Copy link
Contributor

ok

Signed-off-by: Alexander Garagatyi <agaragatyi@codenvy.com>
@garagatyi garagatyi force-pushed the shineDownHttpJsonHelper branch from cc3e3ce to 0985c2f Compare May 4, 2016 11:42
Signed-off-by: Alexander Garagatyi <agaragatyi@codenvy.com>
@garagatyi garagatyi merged commit 263beb3 into master May 4, 2016
@garagatyi garagatyi deleted the shineDownHttpJsonHelper branch May 4, 2016 11:45
@tareksha
Copy link
Contributor

Hi,
can this be down-ported to 3.x ?
@garagatyi @skabashnyuk @sleshchenko @evoevodin

@skabashnyuk
Copy link
Contributor

@tareqhs Yes. If you want you can do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants