Uploaded image for project: 'Java Couchbase JVM Core'
  1. Java Couchbase JVM Core
  2. JVMCBC-569

Switch to /admin/ping endpoint for http keepalive

    XMLWordPrintable

Details

    • Improvement
    • Status: Resolved
    • Critical
    • Resolution: Fixed
    • None
    • 1.7.0
    • analytics
    • None
    • 1
    • CX Sprint 120

    Description

      In our recent system tests performed by QE, there were over 660K calls to the API "/analytics/version". That API was removed, and because of that, each one of these calls is generating the following log and those logs are filling our debug log file:

      No servlet for /analytics/version
      

      I checked with QE and they are using the Java SDK to issue queries. It looks like that API is still being used in AnalyticsHandler for KeepAliveRequests. The usage of that API should be removed and replaced by another API.

      I also noticed that the PingRequest is using the API /admin/ping. To which port is that request being sent? and how often those ping and keep alive requests are performed?

      Attachments

        Issue Links

          No reviews matched the request. Check your Options in the drop-down menu of this sections header.

          Activity

            There is no "alice" on JVMCBC Lynn Straus and now that the problem is the removed API, I'm not sure why Michael Blow moved it back to JVMCBC. Seems that the bug is the removal of the old REST endpoint, and it's being fixed by adding a new one, which means asking for a change in JVMCBC, right?

            Michael Blow: do you have an MB where you're tracking the removal/addition? If so, I can just fix up this one. Otherwise, I'd propose moving it back and adding a tracking JVMCBC

            ingenthr Matt Ingenthron added a comment - There is no "alice" on JVMCBC Lynn Straus and now that the problem is the removed API, I'm not sure why Michael Blow moved it back to JVMCBC. Seems that the bug is the removal of the old REST endpoint, and it's being fixed by adding a new one, which means asking for a change in JVMCBC, right? Michael Blow : do you have an MB where you're tracking the removal/addition? If so, I can just fix up this one. Otherwise, I'd propose moving it back and adding a tracking JVMCBC
            ingenthr Matt Ingenthron added a comment - - edited

            Related Michael Blow, when you're adding the new, efficient REST endpoint, you may want to consider reverting the removal of the old one as any users on current releases would start seeing noise and possibly other problems from this. Obviously they can upgrade to make the problem go away but only after we update the SDK. Generally speaking, it's probably best to not make changes like this after Beta and if we did, best to intro the new interface, notify consumers, then remove the old interface after dependents update.

            I believe this change in analytics will impact not only Java, but also .NET/lcb/go.

            ingenthr Matt Ingenthron added a comment - - edited Related Michael Blow , when you're adding the new, efficient REST endpoint, you may want to consider reverting the removal of the old one as any users on current releases would start seeing noise and possibly other problems from this. Obviously they can upgrade to make the problem go away but only after we update the SDK. Generally speaking, it's probably best to not make changes like this after Beta and if we did, best to intro the new interface, notify consumers, then remove the old interface after dependents update. I believe this change in analytics will impact not only Java, but also .NET/lcb/go.
            murtadha.hubail Murtadha Hubail added a comment - - edited

            Hi Matt Ingenthron,
            I completely agree with you in terms of deprecation and adding new interfaces, but I just want to clarify a couple of things.

            First, the version API was removed even before Analytics Developer Preview 5 (volatile APIs), which was the first ever integrated version of analytics in CB 5.5 release. So, if anything is broken, it has been broken since then. The removal of the API was tracked by MB-29837. I agree that things would've been easier if this was communicated with a wider audience, but as I said in my previous post, we didn't think anyone would use that API, especially in a volatile APIs state that gives us liberty of changing/removing any API before beta/GA. In addition, the Analytics SDK-RFC that you guys shared with us had no mention of using this API. If the SDKs were using the output of that API, any regression test would've caught it. Do SDKs ensure the returned response code must be (200) to decide on the result of keep-alive? if not, then I think even with the API removed, that might still be working. QE has been using the Java SDK for system/longevity tests on Alice without issues and the only noise right now is that generated log. But I would imagine any supported use case of analytics will be with the Alice release so I would expect users to update the SDKs to the version that is fully compatible with Alice before using them with Analytics. Having said that, I don't think there is a need to revert the removal of that API, since it was removed when the APIs were in a volatile state, unless it is breaking the current/older SDKs functionality in a really bad way.

            Second, about the new ping API that was added today. Each ping request generated by the SDK before today was proxied to the analytics master node, which adds an overhead of establishing the proxy connection. The new ping API that was added today eliminates that proxying step and returns the response directly from the contacted node. SDKs will automatically use that newly added API for pings without any changes needed with the Alice release. Unfortunately, we don't have an MB for the addition of the new ping API anymore. I wished that Michael N. just filed a new issue on MB and changed this as a requested change on SDKs with me as the reported instead of moving this one as an MB. Please feel free to move this back as MB-31375 and make sure it is marked as resolved with Alice as the fix version. Please file the required SDKs changes to replace the use of the removed version API for keep-alive requests by the ping API, similar to QueryHandler.

            murtadha.hubail Murtadha Hubail added a comment - - edited Hi Matt Ingenthron , I completely agree with you in terms of deprecation and adding new interfaces, but I just want to clarify a couple of things. First, the version API was removed even before Analytics Developer Preview 5 (volatile APIs), which was the first ever integrated version of analytics in CB 5.5 release. So, if anything is broken, it has been broken since then. The removal of the API was tracked by MB-29837 . I agree that things would've been easier if this was communicated with a wider audience, but as I said in my previous post, we didn't think anyone would use that API, especially in a volatile APIs state that gives us liberty of changing/removing any API before beta/GA. In addition, the Analytics SDK-RFC that you guys shared with us had no mention of using this API. If the SDKs were using the output of that API, any regression test would've caught it. Do SDKs ensure the returned response code must be (200) to decide on the result of keep-alive? if not, then I think even with the API removed, that might still be working. QE has been using the Java SDK for system/longevity tests on Alice without issues and the only noise right now is that generated log. But I would imagine any supported use case of analytics will be with the Alice release so I would expect users to update the SDKs to the version that is fully compatible with Alice before using them with Analytics. Having said that, I don't think there is a need to revert the removal of that API, since it was removed when the APIs were in a volatile state, unless it is breaking the current/older SDKs functionality in a really bad way. Second, about the new ping API that was added today. Each ping request generated by the SDK before today was proxied to the analytics master node, which adds an overhead of establishing the proxy connection. The new ping API that was added today eliminates that proxying step and returns the response directly from the contacted node. SDKs will automatically use that newly added API for pings without any changes needed with the Alice release. Unfortunately, we don't have an MB for the addition of the new ping API anymore. I wished that Michael N. just filed a new issue on MB and changed this as a requested change on SDKs with me as the reported instead of moving this one as an MB. Please feel free to move this back as MB-31375 and make sure it is marked as resolved with Alice as the fix version. Please file the required SDKs changes to replace the use of the removed version API for keep-alive requests by the ping API, similar to QueryHandler.

            Merged https://github.com/couchbase/couchbase-jvm-core/commit/904a76184ca073fa5c7e044378c91c969d752a81 and also added to the SDK RFC for the other implementations to align.

            daschl Michael Nitschinger added a comment - Merged https://github.com/couchbase/couchbase-jvm-core/commit/904a76184ca073fa5c7e044378c91c969d752a81 and also added to the SDK RFC for the other implementations to align.

            Build couchbase-server-6.5.0-1350 contains asterix-opt commit f5a39fc with commit message:
            MB-31375: provide efficient ping api on public port

            build-team Couchbase Build Team added a comment - Build couchbase-server-6.5.0-1350 contains asterix-opt commit f5a39fc with commit message: MB-31375 : provide efficient ping api on public port

            People

              daschl Michael Nitschinger
              murtadha.hubail Murtadha Hubail
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Gerrit Reviews

                  There are no open Gerrit changes

                  PagerDuty